Re: Have ScriptPromiseResolver on the Oilpan heap always. (issue 1233173002 by sigbjornf@opera.com)

[har...@chromium.org]

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v8/ScriptPromiseResolver.h
File Source/bindings/core/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v8/ScriptPromiseResolver.h#newcode147
Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is
needed.
On 2015/07/17 05:34:30, sof wrote:
> On 2015/07/17 01:40:35, haraken wrote:
> >
> > Can we use RefCountedGarbageCollected instead of manually creating a
keep
> alive
> > perssitent? RefCountedGarbageCollected is the way to create the keep
alive
> > persistent in a safe manner.
> >

> I don't agree with that suggestion. Two reasons:

> - there's no counting required here.
> - it exposes reference counting as part of the public interface of
these
> objects; it's an internal implementation detail.

> You'll find instances of local Persistent<> self keep alives
elsewhere.

That is something we need to fix. I'm ok with either plan (i.e., using a
self keep alive handle or using RefCountedGarbageCollected) but we
should avoid mixing the two syntax in the code base.

a) Stop using RefCountedGarbageCollected for the self-keep-alive
purpose. Use an explicit persistent handle.

b) Use RefCountedGarbageCollected for the self-keep-alive purpose.
Rename ref/deref to a better name.

If your plan is to apply a) for all the code base, I'm fine with it :)

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v8/ScriptPromiseResolver.h
File Source/bindings/core/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v8/ScriptPromiseResolver.h#newcode147
Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is
needed.

What/where are they? (I don't know of any, off-hand.) We already have
self-keep alives using Persistent<> in places.

https://codereview.chromium.org/1233173002/

[har...@chromium.org]

SuspendableScriptExecutor is one example (since I was thinking that b) is a
plan). I'm fine with a).



https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

i see, thanks. Added a light abstraction SelfKeepAlive<T> to handle the
details.
(Will adopt its use more fully in a follow-up, if (it&) that's acceptable.)

https://codereview.chromium.org/1233173002/

[har...@chromium.org]

Looks good! (Let me take a look at the rest of the CL later.)


https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/ReadableStreamImpl.h
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/ReadableStreamImpl.h#newcode178
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();

On 2015/07/17 01:40:35, haraken wrote:

> Not related to this CL, we should have:

> if (!scriptState->contextIsValid())
> return false;

Done, here & in resolveAllPendingReadsAsDone()

yhirano@: ptal ?

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachestorage/Cache.cpp
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachestorage/Cache.cpp#newcode33
Source/modules/cachestorage/Cache.cpp:33:
CacheMatchCallbacks(ScriptPromiseResolver* resolver)

On 2015/07/17 01:40:35, haraken wrote:

> Add explicit. Same comment for other constructors in this file.


Done.

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/crypto/CryptoResultImpl.cpp
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/crypto/CryptoResultImpl.cpp#newcode121
Source/modules/crypto/CryptoResultImpl.cpp:121: if
(scriptState->executionContext()->activeDOMObjectsAreStopped()) {
On 2015/07/17 01:40:36, haraken wrote:

> Not related to this CL, why do we need this special logic only in
> CryptoResultImpl? If we need this here, I guess we need it for all
Resolver
> owners.

It's due to
https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/CryptoResultImpl.cpp#newcode198

As CryptoResultImpl now keeps a Member<>, we no longer this special
case. (I thought it was there as a leak-preventing measure, but it was
needed for weak pointer safety.)

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/mediastream/MediaDevices.cpp
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/mediastream/MediaDevices.cpp#newcode41
Source/modules/mediastream/MediaDevices.cpp:41:
On 2015/07/17 01:40:36, haraken wrote:

> I'm just curious, but is it valid that we reach the destructor without
> m_resolver getting resolved? I guess it's not guaranteed that
handleEvent is
> always called.

But then handleEvent() would be called for the paired
PromiseErrorCallback instead?

https://codereview.chromium.org/1233173002/

[yhi...@chromium.org]

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/ReadableStreamImpl.h
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/ReadableStreamImpl.h#newcode178
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();

On 2015/07/17 09:43:26, sof wrote:
> On 2015/07/17 01:40:35, haraken wrote:
> >
> > Not related to this CL, we should have:
> >
> > if (!scriptState->contextIsValid())
> > return false;

> Done, here & in resolveAllPendingReadsAsDone()

> yhirano@: ptal ?

haraken@: Is it guaranteed that the document is stopped when
scriptState->contextIsValid() returns false? Otherwise an assertion in
the ScriptPromiseResolver fails and we should modify there as well.

https://codereview.chromium.org/1233173002/

[har...@chromium.org]

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/ReadableStreamImpl.h
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/ReadableStreamImpl.h#newcode178
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();

On 2015/07/17 13:53:41, yhirano wrote:
> On 2015/07/17 09:43:26, sof wrote:
> > On 2015/07/17 01:40:35, haraken wrote:
> > >
> > > Not related to this CL, we should have:
> > >
> > > if (!scriptState->contextIsValid())
> > > return false;
> >
> > Done, here & in resolveAllPendingReadsAsDone()
> >
> > yhirano@: ptal ?

> haraken@: Is it guaranteed that the document is stopped when
> scriptState->contextIsValid() returns false? Otherwise an assertion in
the
> ScriptPromiseResolver fails and we should modify there as well.

It is guaranteed for a document that is associated with a frame. It is
not guaranteed for some documents that don't have a frame, but that's an
edge case (e.g., XSLT documents).

https://codereview.chromium.org/1233173002/

[har...@chromium.org]

LGTM



https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp#newcode48
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)

Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolverTest.cpp
File Source/bindings/core/v8/ScriptPromiseResolverTest.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolverTest.cpp#newcode227
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:227:
ScriptPromiseResolverKeepAlive(ScriptState* scriptState)

Add explicit.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachestorage/Cache.cpp
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachestorage/Cache.cpp#newcode70
Source/modules/cachestorage/Cache.cpp:70: m_resolver.clear();

Nit: So let's remove these .clear() in follow-up CLs.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachestorage/CacheStorage.cpp
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachestorage/CacheStorage.cpp#newcode100
Source/modules/cachestorage/CacheStorage.cpp:100:
MatchCallbacks(ScriptPromiseResolver* resolver)

Add explicit.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/CryptoResultImpl.cpp
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/CryptoResultImpl.cpp#newcode120
Source/modules/crypto/CryptoResultImpl.cpp:120:
ASSERT(scriptState->contextIsValid());

Nit: It would be nice to move this assert to Resolver::create.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediastream/MediaDevices.cpp
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediastream/MediaDevices.cpp#newcode37
Source/modules/mediastream/MediaDevices.cpp:37:
PromiseSuccessCallback(ScriptPromiseResolver* resolver)

Add explicit.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediastream/MediaDevices.cpp#newcode63
Source/modules/mediastream/MediaDevices.cpp:63:
PromiseErrorCallback(ScriptPromiseResolver* resolver)

Add explicit.

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

Thanks for the (weekend) review.

Until the Blink thread that initiates a crypto operation also take care of
releasing its CryptoResultImpl (heap) result (
https://codereview.chromium.org/1228373006/ ), this CL cannot go ahead. Will
park it until then.

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp#newcode48
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)

On 2015/07/18 08:01:23, haraken wrote:

> Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?

I don't think that's accurate - how do you reason to suggest that here?

i.e., consider MIDIAccessInitializer::start() being called with an
execution context in a suspended state. It'll set m_keepAlive. start()
may then progress and try to call reject() (say), where
ScriptPromiseResolver will call keepAliveWhilePending() again (via
resolveOrReject()). m_keepAlive will then be set.

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolverTest.cpp
File Source/bindings/core/v8/ScriptPromiseResolverTest.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolverTest.cpp#newcode227
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:227:
ScriptPromiseResolverKeepAlive(ScriptState* scriptState)

On 2015/07/18 08:01:23, haraken wrote:

> Add explicit.

Done.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachestorage/CacheStorage.cpp
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachestorage/CacheStorage.cpp#newcode100
Source/modules/cachestorage/CacheStorage.cpp:100:
MatchCallbacks(ScriptPromiseResolver* resolver)

On 2015/07/18 08:01:24, haraken wrote:

> Add explicit.

Done.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/CryptoResultImpl.cpp
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/CryptoResultImpl.cpp#newcode120
Source/modules/crypto/CryptoResultImpl.cpp:120:
ASSERT(scriptState->contextIsValid());

On 2015/07/18 08:01:24, haraken wrote:

> Nit: It would be nice to move this assert to Resolver::create.

Minor nit addressed :)

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediastream/MediaDevices.cpp
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediastream/MediaDevices.cpp#newcode37
Source/modules/mediastream/MediaDevices.cpp:37:
PromiseSuccessCallback(ScriptPromiseResolver* resolver)

On 2015/07/18 08:01:24, haraken wrote:

> Add explicit.

Done.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediastream/MediaDevices.cpp#newcode63
Source/modules/mediastream/MediaDevices.cpp:63:
PromiseErrorCallback(ScriptPromiseResolver* resolver)

On 2015/07/18 08:01:24, haraken wrote:

> Add explicit.

Done.

https://codereview.chromium.org/1233173002/

[har...@chromium.org]

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp#newcode48
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)

On 2015/07/18 20:46:08, sof wrote:
> On 2015/07/18 08:01:23, haraken wrote:
> >
> > Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?

> I don't think that's accurate - how do you reason to suggest that
here?

> i.e., consider MIDIAccessInitializer::start() being called with an
execution
> context in a suspended state. It'll set m_keepAlive. start() may then
progress
> and try to call reject() (say), where ScriptPromiseResolver will call
> keepAliveWhilePending() again (via resolveOrReject()). m_keepAlive
will then be
> set.

Makes sense! Would be worth having a comment -- keepAliveWhilePending
can be called multiple times but those are expected to be cleared with
one clear() call.

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v8/ScriptPromiseResolver.cpp#newcode48
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)

On 2015/07/19 07:25:50, haraken wrote:
> On 2015/07/18 20:46:08, sof wrote:
> > On 2015/07/18 08:01:23, haraken wrote:
> > >
> > > Instead of adding '|| m_keepAlive', can we add
ASSERT(!m_keepAlive)?
> >
> > I don't think that's accurate - how do you reason to suggest that
here?
> >
> > i.e., consider MIDIAccessInitializer::start() being called with an
execution
> > context in a suspended state. It'll set m_keepAlive. start() may
then progress
> > and try to call reject() (say), where ScriptPromiseResolver will
call
> > keepAliveWhilePending() again (via resolveOrReject()). m_keepAlive
will then
> be
> > set.

> Makes sense! Would be worth having a comment -- keepAliveWhilePending
can be
> called multiple times but those are expected to be cleared with one
clear()
> call.

Added clarifying comment.

https://codereview.chromium.org/1233173002/

[yhi...@chromium.org]

core/streams LGTM


https://codereview.chromium.org/1233173002/

[har...@chromium.org]

https://codereview.chromium.org/1233173002/diff/280001/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1233173002/diff/280001/Source/platform/heap/Handle.h#newcode1045
Source/platform/heap/Handle.h:1045: class SelfKeepAlive {

Can we land this (with some HeapTests) ahead of the time?

We want to use the SelfKeepAlive handle in
https://codereview.chromium.org/1257653007/ :)

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

I'd prefer to unblock this by landing the mazy
https://codereview.chromium.org/1228373006/ (it is way overdue.)

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

With https://codereview.chromium.org/1228373006/ having landed, rebased and
updated this one. pta(quick)l?

jochen@: could you have a look at the public/ changes? tia.

https://codereview.chromium.org/1233173002/

[joc...@chromium.org]

public lgtm

https://codereview.chromium.org/1233173002/

[rt...@chromium.org]

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudio/RealtimeAnalyser.cpp
File Source/modules/webaudio/RealtimeAnalyser.cpp (right):

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudio/RealtimeAnalyser.cpp#newcode40
Source/modules/webaudio/RealtimeAnalyser.cpp:40: const double
RealtimeAnalyser::DefaultSmoothingTimeConstant = 0.8;
Drive-by-comment:

This change seems unrelated. Given the large size of the change, I
think it would be better not to do this here, even though it's trivial.

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudio/RealtimeAnalyser.cpp
File Source/modules/webaudio/RealtimeAnalyser.cpp (right):

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudio/RealtimeAnalyser.cpp#newcode40
Source/modules/webaudio/RealtimeAnalyser.cpp:40: const double
RealtimeAnalyser::DefaultSmoothingTimeConstant = 0.8;

On 2015/08/03 15:46:54, Raymond Toy wrote:
> Drive-by-comment:

> This change seems unrelated. Given the large size of the change, I
think it
> would be better not to do this here, even though it's trivial.

A whitespace change too trivial to merit a change of its own, I
reasoned, after stumbling across another one of its kind in a modules/
file.

Could you explain your reasoning for why it would be risky to include it
here?

https://codereview.chromium.org/1233173002/

[rt...@chromium.org]

The only pain is if someone has to look through history to find how
something
changed and has to examine extra files for trivial whitespace changes
unrelated
to the actual change.

I won't oppose this change; I just prefer not seeing unrelated changes,
even if
trivial. :-)

https://codereview.chromium.org/1233173002/

[sigb...@opera.com]

this CL is proving hard to keep working across CLs being landed that creates
ScriptPromiseResolvers; haraken@: still content with the latest (>= ps#12) ?

https://codereview.chromium.org/1233173002/

[har...@chromium.org]

On 2015/08/04 05:20:24, sof wrote:
> this CL is proving hard to keep working across CLs being landed that
> creates
> ScriptPromiseResolvers; haraken@: still content with the latest (>=
> ps#12) ?

Still LGTM


https://codereview.chromium.org/1233173002/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1233173002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #18 (id:420001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199953

https://codereview.chromium.org/1233173002/