Support "await a stable state" and "provide a stable state"

Support "await a stable state" and "provide a stable state"

http://whatwg.org/html#await-a-stable-state
http://whatwg.org/html#provide-a-stable-state

The "await a stable state" concept is used extensively in the spec for
HTMLMediaElement and also for HTMLImageElement. Most notably, it is
required to make the media resource selection algorithm work per spec.

Some instances of "provide a stable state" have been deliberately not
been implemented, pending the outcome of two spec bugs:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24361
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24707

Currently there are no users of StableState::Awaiter, but it is
intended to be used in one of two ways:

 1. Let the client class inherit StableState::Awaiter and receive the
    callback directly. This will likely be sufficient in most cases.

 2. Let a separate StableState::Awaiter subclass keep track of what
    callback to invoke. This may be cleaner if there are many reasons
    to "await a stable state", as no state bits need to be kept in the
    client class itself. The price to pay for this is a WeakPtr or
    similar to deal with the client object being deleted before a
    stable state has been provided.

Other designs considered:

 1. Microtask-like: A set of raw function pointers or closures. This
    would require a WeakPtr to the client object even for simple uses.

 2. Timer-like: A class intended to be used as a member of the client
    class, that binds to a callback on that class. That member would
    require memory even when not awaiting a stable state.

Note that the StableState::provide() call sites are untested as of
this commit. Testing them using a LayoutTest requires an actual user
of StableState::await(), which will be introduced shortly.

BUG=340547

[philipj_slow, 2014-02-04 13:43:01]

Depends on https://codereview.chromium.org/153813002 so no trybots.

I looked at a bunch of different approaches before arriving at this:

1. Something similar to Microtask, which just queues a function pointer (or
Closure) which with no way of removing it from the queue. I rejected this
because the resource selection algorithm in HTMLMediaElement awaits a stable
state and may be aborted, in which case any ongoing awaiting must be stopped.

2. Something similar to Timer, where one has a member Timer<MyClass> m_timer
that's initialized with the callback. However, with stable states there's no
need for a single class to have several awaiters, since sync sections (the thing
that didAwaitStableState does) must be order-independent, so if a single class
can await a stable state for multiple purposes, it can just have flags for that.

Eliminating the need for a member, I ended up with this.

[philipj_slow, 2014-02-04 13:45:46]

https://codereview.chromium.org/153813002/diff/1/Source/core/html/StableState.h
File Source/core/html/StableState.h (right):

https://codereview.chromium.org/153813002/diff/1/Source/core/html/StableState...
Source/core/html/StableState.h:19: void awaitStableState();
I put this here to make the call sites slightly prettier, but it could just as
well be static StableState::await(Awaiter&). That would keep the static bits
together, instead of hiding the fact as I've done now. Should I just change it?

[philipj_slow, 2014-02-04 15:10:48]

https://codereview.chromium.org/153813002/diff/1/Source/core/html/StableState.h
File Source/core/html/StableState.h (right):

https://codereview.chromium.org/153813002/diff/1/Source/core/html/StableState...
Source/core/html/StableState.h:19: void awaitStableState();
On 2014/02/04 13:45:46, philipj wrote:
> I put this here to make the call sites slightly prettier, but it could just as
> well be static StableState::await(Awaiter&). That would keep the static bits
> together, instead of hiding the fact as I've done now. Should I just change
it?

I've made it so in PS2, so you can see exactly what the options are.

[philipj_slow, 2014-02-05 03:53:18]

Another "provide a stable state" was added to the spec in response to my
feedback:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24484
http://www.whatwg.org/specs/web-apps/current-work/#pause

That will require some updates to this CL.

[philipj_slow, 2014-02-05 18:46:54]

PTAL. I've massaged the implementation to my liking and written some background
in the commit message which I hope should help.

[tkent, 2014-02-06 04:27:45]

I have a question.  is ti possible that Code inside StableState::provide
dispatches events and arbitrary JavaScript code runs?

https://codereview.chromium.org/153813002/diff/80001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/153813002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:2520: // http://whatwg.org/html#pause
This comment doesn't explain why we need to pause here.

[philipj_slow, 2014-02-06 05:20:25]

On 2014/02/06 04:27:45, tkent wrote:
> I have a question.  is ti possible that Code inside StableState::provide
> dispatches events and arbitrary JavaScript code runs?

No, none of the synchronous sections in the spec do that, and there's a note
that says "A synchronous section never mutates the DOM, runs any script, or has
any side-effects detectable from another synchronous section, and thus
synchronous sections can be run in any order, and cannot spin the event loop."

>
https://codereview.chromium.org/153813002/diff/80001/Source/core/dom/Document...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/153813002/diff/80001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:2520: // http://whatwg.org/html#pause
> This comment doesn't explain why we need to pause here.

It's part of <http://whatwg.org/html#prompt-to-unload-a-document>, should I link
to that? Document::dispatchBeforeUnloadEvent doesn't quite match the spec, so
I'm not sure where to put the comment.

[philipj_slow, 2014-02-06 05:41:26]

I realized that "Calling await() inside the callback has no effect." was only
true if the same awaiter was used, so I changed it to just not be allowed, since
the spec never does this anyway.

[philipj_slow, 2014-02-07 06:50:18]

Aaron, you may be interested in this as well, since I intend to use it in
HTMLMediaElement.

[acolwell GONE FROM CHROMIUM, 2014-02-07 19:09:30]

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
File Source/core/html/StableState.cpp (right):

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableState.cpp:16: DEFINE_STATIC_LOCAL(StableStateAwaiters,
stableStateAwaiters, ());
Do we really want this process global? Shouldn't this be thread local since I
believe we can have multiple documents in a single process? I must admit, I'm
not overly familiar with this part of the codebase so I apologize in advance if
these are silly questions.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableState.cpp:27: stableStateAwaiters().add(&awaiter);
nit: I wonder if we should keep a bool that keeps track of whether we are inside
a provide() call and ASSERT() on it here. It would be better to crash in the
actual invalid call instead of after the fact like the ASSERT() below.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableState.cpp:32: if (!stableStateAwaiters().isEmpty()) {
nit: Reverse condition and early return.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
File Source/core/html/StableStateTest.cpp (right):

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableStateTest.cpp:102: StableState::await(*new
ClosureAwaiter(bind(&MockCallbacks::callback1, &callbacks)));
I don't quite get what this is testing. Why do you need the new/delete business
here? Can't you just use local variables like you do in the tests above?

[philipj_slow, 2014-02-09 17:10:17]

PTAL. I'm going on a short vacation starting Wednesday, so it'd be great to have
this finished before then.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
File Source/core/html/StableState.cpp (right):

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableState.cpp:16: DEFINE_STATIC_LOCAL(StableStateAwaiters,
stableStateAwaiters, ());
On 2014/02/07 19:09:31, acolwell wrote:
> Do we really want this process global? Shouldn't this be thread local since I
> believe we can have multiple documents in a single process? I must admit, I'm
> not overly familiar with this part of the codebase so I apologize in advance
if
> these are silly questions.

In the spec these concepts are not scoped per document, or anything at all. Of
course not all documents are reachable for all other documents, so there may be
ways to scope it that is indistinguishable in testing from a global StableState
instance.

That being said, I believe it is safe to have a global instance. These functions
should only be used from the main thread (asserts added) and because the
awaiters are run at the end of the tasks, the list of awaiters should be empty
before any other top-level browsing context ("tab") gets to see it.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableState.cpp:27: stableStateAwaiters().add(&awaiter);
On 2014/02/07 19:09:31, acolwell wrote:
> nit: I wonder if we should keep a bool that keeps track of whether we are
inside
> a provide() call and ASSERT() on it here. It would be better to crash in the
> actual invalid call instead of after the fact like the ASSERT() below.

Done.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableState.cpp:32: if (!stableStateAwaiters().isEmpty()) {
On 2014/02/07 19:09:31, acolwell wrote:
> nit: Reverse condition and early return.

Done.

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
File Source/core/html/StableStateTest.cpp (right):

https://codereview.chromium.org/153813002/diff/170001/Source/core/html/Stable...
Source/core/html/StableStateTest.cpp:102: StableState::await(*new
ClosureAwaiter(bind(&MockCallbacks::callback1, &callbacks)));
On 2014/02/07 19:09:31, acolwell wrote:
> I don't quite get what this is testing. Why do you need the new/delete
business
> here? Can't you just use local variables like you do in the tests above?

This is verifying the "separate StableState::Awaiter subclass" use case
mentioned in the commit message. I've expanded on the comment above to clarify.
It's easy to screw up delete during iteration, for example if one uses a
Vector<StableState::Awaiter*> and StableState::provide() iterates it directly
(no swap), all previous tests pass but this one crashes.

[acolwell GONE FROM CHROMIUM, 2014-02-10 22:36:05]

This is looking pretty good to me. You should have someone more familiar w/ the
Document, DOMWindow, and HTMLScriptRunner changes take a look as well. While the
changes are trivial, I'm not an expert in this area and don't feel like my owner
rights apply to those files.

https://codereview.chromium.org/153813002/diff/250001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:1042: StableState::provide();
nit: You should probably find a good home for the pause concept and make a call
to that here and elsewhere. I think that would make it easier to update the
pause logic in the future if necessary.

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
File Source/core/html/StableState.cpp (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.cpp:21: static bool isInStableState = false;
nit: Now that we have more than one piece of state for this, I think the "static
thing" should just be a StableState object and the awaiters and bool be members.

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.cpp:47: awaiters.swap(stableStateAwaiters());
Can't this cause a use-after-free if executing didAwaitStableState() on one
awaiter causes the destruction of another awaiter?

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
File Source/core/html/StableState.h (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.h:15: virtual void didAwaitStableState() = 0;
nit: didAwaitStableState seems like an awkward name. How about something like
onStableState, reachedStableState, awaitDone, or awaitComplete?

[philipj_slow, 2014-02-11 17:25:21]

I'm going to be gone for a few days, so I'll have to put this on hold until next
week.

https://codereview.chromium.org/153813002/diff/250001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:1042: StableState::provide();
On 2014/02/10 22:36:05, acolwell wrote:
> nit: You should probably find a good home for the pause concept and make a
call
> to that here and elsewhere. I think that would make it easier to update the
> pause logic in the future if necessary.

Yeah, I started looking for a place and a name but couldn't find one so I gave
up. I don't think it's very important to provide a stable state here, see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24484#c2

Since it creates an extra testing effort (that can't be automated cross-browser
because the dialogs are blocking) maybe I should push harder to have "await a
stable state" dropped from "pause"? Even if the spec doesn't change, we can just
wait with implementing this bit until some other browser does first. Yes?

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
File Source/core/html/StableState.h (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.h:15: virtual void didAwaitStableState() = 0;
On 2014/02/10 22:36:05, acolwell wrote:
> nit: didAwaitStableState seems like an awkward name. How about something like
> onStableState, reachedStableState, awaitDone, or awaitComplete?

I rather like my name, actually, it blends well with other things in
HTMLMediaElement like didMoveToNewDocument, didRecalcStyle, didAddTrack,
didRemoveTrack and didBecomeFullscreenElement.

Of course, if you pick a specific name you would prefer, I will make it so.

[philipj_slow, 2014-02-17 11:59:24]

OK, I'm now mostly happy with this.

Please let me know if we can remove the http://whatwg.org/html#pause steps for
dialogs, in which case I will argue for that spec change and follow up with
whatever change is needed if I don't get my way.

Note also that once https://codereview.chromium.org/169093004/ is landed I would
like to put StableState::provide() inside if (!isExecutingScript()), again in
violation of the spec.

In short, I would like an "await a stable state" concept that means "wait until
all currently running scripts have finished running, until a script is about to
run, or until the end of the current task".

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
File Source/core/html/StableState.cpp (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.cpp:21: static bool isInStableState = false;
On 2014/02/10 22:36:05, acolwell wrote:
> nit: Now that we have more than one piece of state for this, I think the
"static
> thing" should just be a StableState object and the awaiters and bool be
members.

Then the call sites would have to say something like
"StableState::instance().provide()", see if you like my alternative approach
with an internal StableStateAwaiters object.

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.cpp:47: awaiters.swap(stableStateAwaiters());
On 2014/02/10 22:36:05, acolwell wrote:
> Can't this cause a use-after-free if executing didAwaitStableState() on one
> awaiter causes the destruction of another awaiter?

Yes, indeed it can. Since the callbacks aren't run in a deterministic order and
I can't think of a reason to allow it right now, I'll disallow deleting another
awaiter in the callback and assert that it is so.

[philipj_slow, 2014-02-18 10:30:54]

rafaelw, would you mind taking a look at this, since you are familiar with the
microtask concept? The commit message should tell you everything you need to
know, including why the PS7 is named "violate the spec". Thanks!

[acolwell GONE FROM CHROMIUM, 2014-02-18 18:47:42]

lgtm % nits

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
File Source/core/html/StableState.h (right):

https://codereview.chromium.org/153813002/diff/250001/Source/core/html/Stable...
Source/core/html/StableState.h:15: virtual void didAwaitStableState() = 0;
On 2014/02/11 17:25:21, philipj wrote:
> On 2014/02/10 22:36:05, acolwell wrote:
> > nit: didAwaitStableState seems like an awkward name. How about something
like
> > onStableState, reachedStableState, awaitDone, or awaitComplete?
> 
> I rather like my name, actually, it blends well with other things in
> HTMLMediaElement like didMoveToNewDocument, didRecalcStyle, didAddTrack,
> didRemoveTrack and didBecomeFullscreenElement.
> 
> Of course, if you pick a specific name you would prefer, I will make it so.

You're right. I retract my objection. I'm fine with keeping this as is.

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
File Source/core/html/StableState.cpp (right):

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
Source/core/html/StableState.cpp:14: private:
nit: public declarations should be before private ones.

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
File Source/core/html/StableStateTest.cpp (right):

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
Source/core/html/StableStateTest.cpp:77: // without having state bites by using
a self-deleting StableState::Awaiter
nit: What do you mean by "state bites"? s/bites// or s/bites/bytes/ ?

[rafaelw, 2014-02-19 01:19:43]

I'm trying to convince hixie that the await/provide a stable state in the spec
should just be refactored to use microtasks.

regardless of whether i convince him, we should implement it that way. clients
who now want to "await a stable state", should just enqueue a microtask.

note that another place the spec will differ is alert() which is currently
supposed to provide a stable state. this is a misfeature and we should not
implement it.

[philipj_slow, 2014-02-19 04:10:56]

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
File Source/core/html/StableState.cpp (right):

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
Source/core/html/StableState.cpp:14: private:
On 2014/02/18 18:47:43, acolwell wrote:
> nit: public declarations should be before private ones.

Done.

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
File Source/core/html/StableStateTest.cpp (right):

https://codereview.chromium.org/153813002/diff/600001/Source/core/html/Stable...
Source/core/html/StableStateTest.cpp:77: // without having state bites by using
a self-deleting StableState::Awaiter
On 2014/02/18 18:47:43, acolwell wrote:
> nit: What do you mean by "state bites"? s/bites// or s/bites/bytes/ ?

I meant state bits, stuff like HTMLMediaElement::m_pendingActionFlags.

[philipj_slow, 2014-02-19 04:40:59]

On 2014/02/19 01:19:43, rafaelw wrote:
> I'm trying to convince hixie that the await/provide a stable state in the spec
> should just be refactored to use microtasks.

That's https://www.w3.org/Bugs/Public/show_bug.cgi?id=24484#c6 for those
following from home.

> regardless of whether i convince him, we should implement it that way. clients
> who now want to "await a stable state", should just enqueue a microtask.

In the spec, there are two placed that perform a microtask checkpoint that do
not provide a stable state:

http://www.whatwg.org/specs/web-apps/current-work/#clean-up-after-running-a-c...
http://www.whatwg.org/specs/web-apps/current-work/#spin-the-event-loop

In Blink, there are also two such places:
V8RecursionScope::didLeaveScriptContext and
HTMLScriptRunner::executePendingScriptAndDispatchEvent. I'm not sure what step
in the spec they correspond to, but neither seems to be "spin the event loop".

Are you saying that you want me to implement StableState using Microtask, or to
just use "enqueue a microtask" even though it doesn't quite match "await a
stable state"? I would prefer to go with what I have implemented and evolve it
if and when the spec changes. I have work in HTMLMediaElement blocked on this so
please be explicit so that we can resolve this with as few roundtrips as
possible. (I'm in UTC+7.)

> note that another place the spec will differ is alert() which is currently
> supposed to provide a stable state. this is a misfeature and we should not
> implement it.

I agree, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=24707

[rafaelw1, 2014-02-19 05:01:13]

On Tue, Feb 18, 2014 at 8:40 PM, <philipj@opera.com> wrote:

> On 2014/02/19 01:19:43, rafaelw wrote:
>
>> I'm trying to convince hixie that the await/provide a stable state in the
>> spec
>> should just be refactored to use microtasks.
>>
>
> That's https://www.w3.org/Bugs/Public/show_bug.cgi?id=24484#c6 for those
> following from home.
>
>
>  regardless of whether i convince him, we should implement it that way.
>> clients
>> who now want to "await a stable state", should just enqueue a microtask.
>>
>
> In the spec, there are two placed that perform a microtask checkpoint that
> do
> not provide a stable state:
>
> http://www.whatwg.org/specs/web-apps/current-work/#clean-
> up-after-running-a-callback
> http://www.whatwg.org/specs/web-apps/current-work/#spin-the-event-loop
>
> In Blink, there are also two such places:
> V8RecursionScope::didLeaveScriptContext and
> HTMLScriptRunner::executePendingScriptAndDispatchEvent. I'm not sure what
> step
> in the spec they correspond to, but neither seems to be "spin the event
> loop".
>
> Are you saying that you want me to implement StableState using Microtask,
> or to
> just use "enqueue a microtask" even though it doesn't quite match "await a
>

The later. Basically don't land this patch. For clients who would have
"awaited a stable state" -- have them instead enqueue a microtask.

I realize the spec differs here, but whatwg's charter is to spec what
browsers actually do. The only reason I've heard to have "await a stable
state" be different from running a microtask is the load/alert use case --
which is simply not compelling enough to introduce a second "things have
calmed down" signal within blink.



> stable state"? I would prefer to go with what I have implemented and
> evolve it
> if and when the spec changes. I have work in HTMLMediaElement blocked on
> this so
> please be explicit so that we can resolve this with as few roundtrips as
> possible. (I'm in UTC+7.)
>
>
>  note that another place the spec will differ is alert() which is currently
>> supposed to provide a stable state. this is a misfeature and we should not
>> implement it.
>>
>
> I agree, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=24707
>
> https://codereview.chromium.org/153813002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[philipj_slow, 2014-02-19 07:17:30]

On 2014/02/19 05:01:13, rafaelw1 wrote:
> On Tue, Feb 18, 2014 at 8:40 PM, <mailto:philipj@opera.com> wrote:
> 
> > Are you saying that you want me to implement StableState using Microtask,
> > or to
> > just use "enqueue a microtask" even though it doesn't quite match "await a
> >
> 
> The later. Basically don't land this patch. For clients who would have
> "awaited a stable state" -- have them instead enqueue a microtask.
> 
> I realize the spec differs here, but whatwg's charter is to spec what
> browsers actually do. The only reason I've heard to have "await a stable
> state" be different from running a microtask is the load/alert use case --
> which is simply not compelling enough to introduce a second "things have
> calmed down" signal within blink.

Hixie doesn't always do as he's asked by implementors -- fortunately, I guess --
but let's give it a try. I can't immediately see a reason why the concepts
cannot be merged.

[philipj_slow, 2014-02-21 04:23:46]

I'm closing this until these spec bugs have been resolved:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24361
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24707
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24724

If it can be revived with modifications, I'll do just that.

[cbiesinger, 2014-04-02 22:48:18]

So, it turns out I'd like to use a stable state concept for
https://codereview.chromium.org/200923002/

Could you summarize the current state of stable states and how this CL relates?

[philipj_slow, 2014-04-03 19:36:08]

On 2014/04/02 22:48:18, cbiesinger wrote:
> So, it turns out I'd like to use a stable state concept for
> https://codereview.chromium.org/200923002/
> 
> Could you summarize the current state of stable states and how this CL
relates?

As a result of this CL I filed the above linked spec bugs.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24724 is the one which is
currently blocking progress, so if you want things to move along you may need to
bribe Hixie. I think it's clear that it stable states will be defined in terms
of microtasks in some manner, but there are different ways to define it that
would have observably different results, so I was just going to wait it out.