Add a helper for implementing Promise-valued properties.

Add a helper for implementing Promise-valued properties.

ScriptPromiseProperty is intended for implementing DOM attributes
whose value is a Promise. Specifically:

- The same Promise object is returned each time. Many specifications
  require this (for example FontFaceSet.ready [1]).

- All references to script objects are 'weak'. This means
  ScriptPromiseProperty will not cause leaks through cross-heap
  references. The caller must ensure the lifetime of the "holder's"
  wrapper, which is typically already the case if the property was
  accessed through script.

- The API is designed to support multiple worlds, with one Promise in
  each. However currently only the main world is supported.

This is ongoing work in implementing navigator.serviceWorker.ready.

[1] http://dev.w3.org/csswg/css-font-loading/#font-face-set-ready

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

[dominicc (has gone to gerrit), 2014-07-01 03:51:25]

PTAL

This is the ScriptPromiseProperty part of navigator.serviceWorker.ready, split
out into its own patch.

[yhirano, 2014-07-01 04:38:14]

lgtm

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

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromise.h:103: return !operator==(value);
No need for the parentheses?

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
File Source/bindings/v8/ScriptPromisePropertyBase.h (right):

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromisePropertyBase.h:29: State state() { return
m_state; }
const

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

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromisePropertyTest.cpp:109: EXPECT_EQ(v, w);
EXPECT_FALSE(v.isEmpty())

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromisePropertyTest.cpp:109: EXPECT_EQ(v, w);
State check?

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromisePropertyTest.cpp:121: EXPECT_EQ(v, w);
EXPECT_FALSE(v.isEmpty())

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromisePropertyTest.cpp:121: EXPECT_EQ(v, w);
State check?

[dominicc (has gone to gerrit), 2014-07-01 05:32:16]

Added assertions.

I removed the separate state test; state is better covered by the other tests
now.

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

https://codereview.chromium.org/361863003/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromise.h:103: return !operator==(value);
On 2014/07/01 04:38:14, yhirano wrote:
> No need for the parentheses?

I think they're needed for the explicit invocation of the operator==? I'm
basically cribbing from ScriptValue here.

[haraken, 2014-07-01 06:29:14]

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromiseProperty.h (right):

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseProperty.h:11: #include
"platform/heap/Visitor.h"

We normally include platform/heap/Handle.h.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseProperty.h:95: visitor->trace(m_rejected);

What are types of m_holder, m_resolved and m_rejected?

I'm a bit concerned that if someone stores unexpected type to them, oilpan will
get confused and do some unexpected tracing. As I commented in
ScriptPromisePropertyBase.h, if there is no strong reason to make
ScriptPromisePropertyBase garbage-collected, I'd keep it ref-counted at the
moment.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:34: ScriptState::Scope
scope(ScriptState::from(m_resolver.newLocal(isolate)->CreationContext()));

It would be nicer to explicitly pass in the ScriptState that created the
PendingAction, cache the ScriptState on PendingAction, and use the ScriptState
here.

Then you won't need the HandleScope above.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:38: m_value.clear();

Just to confirm: Even if we don't clear m_value and m_resolver, it won't cause
leaks, right?

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:76: m_mainWorldWrapper.clear();

You don't need to call this, since it's destructed anyway.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:92: ASSERT(executionContext()
&& !executionContext()->activeDOMObjectsAreStopped());

How is this guaranteed?

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:100: ASSERT(m_isolate ==
toIsolate(executionContext()));

You can remove this, since it must hold :)

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:131:
V8HiddenValue::setHiddenValue(m_isolate, promise, promiseName, wrapper);

Add comments (probably in a header file) about how hidden values are organized.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:206: setPendingActivity(this);

setPendingActivity() is deprecated, and we don't want to increase # of call
sites.

Can we make ScriptPromisePropertyBase override
ActiveDOMObject::hasPendingActivity? Probably the hasPendingActivity can return
true if m_pending.size() > 0.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:212:
ASSERT_WITH_SECURITY_IMPLICATION(!m_inResume);

Is there any reason you want to use _WITH_SECURITY_IMPLICATION? Otherwise, let's
just use ASSERT or RELEASE_ASSERT.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:218: while (i <
m_pending.size() && executionContext() &&
!executionContext()->activeDOMObjectsAreSuspended() &&
!executionContext()->activeDOMObjectsAreStopped())

Add a comment and mention that the following execute() can stop/suspend the
execution context.

BTW, how can we guarantee that resume() is not reentered? In other words, I
wonder how it's guaranteed that ASSERT(!m_inResume) holds.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:221: m_pending.remove(0, i);

Probably you can use a linked list for m_pending and use takeFirst().

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromisePropertyBase.h (right):

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.h:20: class
ScriptPromisePropertyBase : public ActiveDOMObject, public
RefCountedWillBeGarbageCollected<ScriptPromisePropertyBase> {

I'm just curious but is there any benefit in making ScriptPromisePropertyBase
garbage-collected? We've not yet moved any binding classes into oilpan's heap.

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.h:54: ScopedPersistent<v8::String>
m_promiseName;

How about just caching (WebCore) Strings instead of
ScopedPersistent<v8::String>s? You can convert the (WebCore) Strings to
v8::Strings with v8String(m_string) whenever you need it. v8String() is fast.

[dominicc (has gone to gerrit), 2014-07-02 04:02:17]

PTAL

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:38: m_value.clear();
On 2014/07/01 06:29:13, haraken wrote:
> 
> Just to confirm: Even if we don't clear m_value and m_resolver, it won't cause
> leaks, right?

Those are strong, so while the ScriptPromiseProperty has pending actions it will
leak if the ScriptPromiseProperty is leaked.

Clearing them here doesn't make much sense because the whole PendingAction
should be deleted soon anyway...

https://codereview.chromium.org/361863003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromisePropertyBase.cpp:92: ASSERT(executionContext()
&& !executionContext()->activeDOMObjectsAreStopped());
On 2014/07/01 06:29:12, haraken wrote:
> 
> How is this guaranteed?

It should be guaranteed by the caller.

Looking at likely users--FontFaceSet, ServiceWorkerContainer--they are already
lifetime-observing (ActiveDOMObject or ContextLifecycleObserver, could become
ActiveDOMObject).

Maybe it is better if I take the ActiveDOMObject and pending stuff out of this
class? Probably the user has to do it anyway. It would make
ScriptPromiseProperty much simpler. WDYT?

[haraken, 2014-07-02 09:04:32]

Thanks for updating the CL! Mostly looks good.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromiseProperty.h (right):

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:19: // attribute whose value is
a Promise, and the same Promise must be

whose value is a Promise => that returns a Promise

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:25: // with the property is
alive. This is achieved by keeping a weak

the wrapper for the object with the property => the holder's wrapper

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:81: return property.release();

return adoptRef(new ScriptPromiseProperty(...));

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:96: if (!(executionContext() &&
state() == Pending))

Is it valid that resolve() is called while state() is not Pending? I wonder if
this could be ASSERT(state() == Pending).

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:99: settle(Resolved);

settle => resolveOrReject (to align with the function name in
ScriptPromiseResolver)

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:115: v8::Handle<v8::Value> value
= V8ValueTraits<HolderType>::toV8Value(m_holder, creationContext, isolate);

Is it guaranteed that we're in a correct context when you call this toV8Value?
If it's not guaranteed, we need to enter a ScriptState::Scope before calling
toV8Value.

I'm fine with the rule that the caller side of these methods should be in a
correct context. Either way, we want to be clear about the rule; I'm a bit
concerned that the current Promise implementation is unclear about the rule and
it might lead to cross-world leakage.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:28: v8::HandleScope
handleScope(m_isolate);

The HandleScope is not needed.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:30: m_promiseName =
makeName(name, "/Promise");

It's a bit fragile that there is no mechanism to prevent name conflict (then
something unexpected/dangerous will happen).

How about forcing developers to list up the names in V8HiddenValue.h?

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:118: return; // wrapper
has died or was never populated

Is this a valid state? In order to provide deterministic behavior to JavaScript,
I think we need to make sure that the holder wrapper is kept alive as long as
the Promise alive.

If so, probably we can add a flag that is set when the weak callback is called,
and assert that the flag is not set here.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:124:
V8HiddenValue::deleteHiddenValue(m_isolate, wrapper, resolverName);

Can we delete the hidden value for the Promise as well?

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:128: void
ScriptPromisePropertyBase::resolveOrReject(v8::Handle<v8::Promise::Resolver>
resolver)

Shall we rename settle to resolveOrReject, and resolveOrReject to
resolveOrRejectInternal? We want to use resolveOrReject for an exposed method.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromisePropertyBase.h (right):

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.h:11: #include "wtf/OwnPtr.h"

This is not needed.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.h:13: #include "wtf/Vector.h"

This is not needed.

[dominicc (has gone to gerrit), 2014-07-03 02:13:40]

PTAL

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromiseProperty.h (right):

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:96: if (!(executionContext() &&
state() == Pending))
On 2014/07/02 09:04:31, haraken wrote:
> 
> Is it valid that resolve() is called while state() is not Pending? I wonder if
> this could be ASSERT(state() == Pending).

I originally did it this way, but yhirano says this should not assert because JS
promises don't.

But I think we should revisit this. C++ != JavaScript; trying to settle the
Promise twice indicates a bug. I've put this back to asserting.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperty.h:115: v8::Handle<v8::Value> value
= V8ValueTraits<HolderType>::toV8Value(m_holder, creationContext, isolate);
On 2014/07/02 09:04:31, haraken wrote:
> 
> Is it guaranteed that we're in a correct context when you call this toV8Value?
> If it's not guaranteed, we need to enter a ScriptState::Scope before calling
> toV8Value.

Yes, the ScriptPromisePropertyBase::promise and ::resolveOrReject handle this. I
added a comment block to the declarations of these in ScriptPromisePropertyBase.

> I'm fine with the rule that the caller side of these methods should be in a
> correct context. Either way, we want to be clear about the rule; I'm a bit
> concerned that the current Promise implementation is unclear about the rule
and
> it might lead to cross-world leakage.

This is why we need ScriptPromiseProperty. If you use ScriptPromiseResolver and
try to cache the Promise it is easy to make a mistake and return a Promise set
up in the first caller's context (because everyone is copy-pasting
CallWith=ScriptState.) Instead the Promise should be in the context associated
with the holder.

You can see ScriptPromisePropertyBase do this in ::promise and
::resolveOrReject. The first thing ::promise does is enter the context for
(holder's execution context, caller's world). ::resolveOrReject is different
because when we support multiple worlds we have to loop through those worlds. So
it enters the context for (cached wrapper's creation context).

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:118: return; // wrapper
has died or was never populated
On 2014/07/02 09:04:32, haraken wrote:
> 
> Is this a valid state? In order to provide deterministic behavior to
JavaScript,
> I think we need to make sure that the holder wrapper is kept alive as long as
> the Promise alive.
> 
> If so, probably we can add a flag that is set when the weak callback is
called,
> and assert that the flag is not set here.

We don't want to prevent garbage collection! So if we made this a strong
reference, we would need a protocol to say when the object is "stopped." It gets
more complicated in the isolated world case--we don't want to prevent an
isolated world's context from being collected, even if the property is still
"alive" in the main world.

A weak reference to the wrapper works for this. Because you access properties
through the holder (eg navigator.serviceWorker.ready) and the holder is already
being kept alive (window -> navigator -> etc. in the ServiceWorkerContainer
case) then we have at least the lifetime we need and no leaks.

Obviously there is a risk that if the holder is leaked for other reasons, or
there's time while the world is dying but we didn't GC yet, we will settle its
Promise. But having an explicit shut-down protocol does not make those problems
go away.

https://codereview.chromium.org/361863003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:124:
V8HiddenValue::deleteHiddenValue(m_isolate, wrapper, resolverName);
On 2014/07/02 09:04:31, haraken wrote:
> 
> Can we delete the hidden value for the Promise as well?

No. We need to keep the reference to the Promise because somebody may get the
Promise object (eg through navigator.serviceWorker.ready) after it has been
settled and we need to return the same object to them, too. Accessing the
Promise after settling is a valid, maybe even typical, use case.

[haraken, 2014-07-03 02:43:20]

LGTM with comments.

https://codereview.chromium.org/361863003/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromiseProperties.h (right):

https://codereview.chromium.org/361863003/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromiseProperties.h:9: #define
SCRIPT_PROMISE_PROPERTIES(P, ...) \

Can we put this into V8HiddenValue.h, instead of adding a new file?

https://codereview.chromium.org/361863003/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/361863003/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:75:
V8HiddenValue::setHiddenValue(m_isolate, promise, promiseName(), wrapper);

Just help me understand: Why do you need the reference from the promise back to
the wrapper?

Don't we need to delete the reference in ~ScriptPromisePropertyBase().

https://codereview.chromium.org/361863003/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:126:
v8::Handle<v8::String> ScriptPromisePropertyBase::promiseName()

I'd move this to V8HiddenValue.cpp.

[dominicc (has gone to gerrit), 2014-07-03 03:16:42]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-07-03 03:17:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominicc@chromium.org/361863003/100001

[commit-bot: I haz the power, 2014-07-03 04:23:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28869)

[commit-bot: I haz the power, 2014-07-03 06:29:53]

Change committed as 177450

[dominicc (has gone to gerrit), 2014-07-03 06:42:42]

On 2014/07/03 06:29:53, I haz the power (commit-bot) wrote:
> Change committed as 177450

Oops. I forgot to remove the part about ActiveDOMObject from the commit message.

[sof, 2014-07-03 13:53:47]

Is causing some unit test failures, revert?


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%2...

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...

[sof, 2014-07-03 14:52:55]

On 2014/07/03 13:53:47, sof wrote:
> Is causing some unit test failures, revert?
> 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%2...
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...

If it is reverted, the http://crrev.com/367263004 followup goes along with it.

[dominicc (has gone to gerrit), 2014-07-04 00:11:22]

On 2014/07/03 14:52:55, sof wrote:
> On 2014/07/03 13:53:47, sof wrote:
> > Is causing some unit test failures, revert?
> > 
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%2...
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...
> 
> If it is reverted, the http://crrev.com/367263004 followup goes along with it.

This should be an easy one, let me send a fix for review:
https://codereview.chromium.org/364293003

[haraken, 2014-07-07 01:04:47]

On 2014/07/04 00:11:22, dominicc wrote:
> On 2014/07/03 14:52:55, sof wrote:
> > On 2014/07/03 13:53:47, sof wrote:
> > > Is causing some unit test failures, revert?
> > > 
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%2...
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...
> > 
> > If it is reverted, the http://crrev.com/367263004 followup goes along with
it.
> 
> This should be an easy one, let me send a fix for review:
> https://codereview.chromium.org/364293003

The unit test added by this CL is still failing in oilpan builds.
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/...

Would you take a look when you have a cycle?