Bug 71242 – [chromium] WebKitMutationObserver::deliverAllMutations should be exposed through the Chromium API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Adam Klein]

For MutationObservers, besides the delivery at the end-of-microtask, there are also cases where we need to deliver at the end of a task in a case where there was no microtask (no script) involved in a DOM mutation (e.g., when user input triggers a DOM modification inside a contentEditable element).  The embedder is in the right position to act at the end of every task, while WebCore is not.  This means exposing WebKitMutationObserver::deliverAllMutations() through the API.

The main question here is one of naming: should this hook be something like WebMutationObserver::deliverAllMutations()?  Or would we be better off with runEndOfTaskWork() (what class would export that method?).

[Darin Fisher (:fishd, Google)]

I'd prefer a more generalized micro-task hook API.  Also, why not always dispatch micro-tasks this way?  Why implement a hybrid approach at all?

One idea is to just move the queue of micro-tasks to the embedder, to the definition of WebThread, so you can do:

  WebKitPlatformSupport::currentThread()->post{Micro}Task(task);

I'm not quite sure how to name this method.  I think postMicroTask is probably not the best option since that name isn't very descriptive.  It is just jargon.  Other ideas:

  postHighPriorityTask - tells you that these run before tasks produced by postTask, but it doesn't indicate that it runs before everything else too.

  postContinuationOfCurrentTask - long winded, but draws a connection to the current task, which seems helpful.

  appendToCurrentTask - a variation of the above.  maybe "post" is better reserved for normal queuing?  hmm, not sure.

At any rate, the behavior of the above methods would be to simply append to a FIFO queue that will be drained from TaskObserver::DidProcessTask on the Chromium side.

I can also imagine something like runEndOfTaskWork() working too.  You could invent a class name to contain that method or perhaps it could just go in WebKit.h.

[Adam Klein]

(In reply to comment #1)
> I'd prefer a more generalized micro-task hook API.  Also, why not always dispatch micro-tasks this way?  Why implement a hybrid approach at all?

There are several reasons. For one example, consider event-handling: when an event fires, multiple listeners may be called during the same task, so those that run after another listener has modified the DOM would get a chance to see it in an inconsistent state.  Running at the end of microtask means that each event listener will have its DOM mutations delivered to MutationObservers before the next one runs.

> 
> One idea is to just move the queue of micro-tasks to the embedder, to the definition of WebThread, so you can do:
> 
>   WebKitPlatformSupport::currentThread()->post{Micro}Task(task);
> 
> I'm not quite sure how to name this method.  I think postMicroTask is probably not the best option since that name isn't very descriptive.  It is just jargon.  Other ideas:
> 
>   postHighPriorityTask - tells you that these run before tasks produced by postTask, but it doesn't indicate that it runs before everything else too.
> 
>   postContinuationOfCurrentTask - long winded, but draws a connection to the current task, which seems helpful.
> 
>   appendToCurrentTask - a variation of the above.  maybe "post" is better reserved for normal queuing?  hmm, not sure.
> 
> At any rate, the behavior of the above methods would be to simply append to a FIFO queue that will be drained from TaskObserver::DidProcessTask on the Chromium side.

I think I like this approach, as it reduces to zero the cost of handling the (quite common) case where no DOM mutations occur during a Task. I'll look into this...

> 
> I can also imagine something like runEndOfTaskWork() working too.  You could invent a class name to contain that method or perhaps it could just go in WebKit.h.

[Adam Klein]

(In reply to comment #2)
> I think I like this approach, as it reduces to zero the cost of handling the (quite common) case where no DOM mutations occur during a Task. I'll look into this...

Having looked into it, it's not clear that this is viable (especially considering that it won't be able to be generalized for microtasks as well as tasks), so I'm going to move ahead with runEndOfTaskWork() for now.

[Adam Klein]

Created attachment 115432 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> (In reply to comment #2)
> > I think I like this approach, as it reduces to zero the cost of handling the (quite common) case where no DOM mutations occur during a Task. I'll look into this...
> 
> Having looked into it, it's not clear that this is viable (especially considering that it won't be able to be generalized for microtasks as well as tasks), so I'm going to move ahead with runEndOfTaskWork() for now.

Can you describe the problem further?  It seems like this is just about maintaining a queue of tasks to run after the current task completes.  Maybe it actually needs to be a stack of queues in order to handle the case of further microtasks generated during the processing of a microtask.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > I think I like this approach, as it reduces to zero the cost of handling the (quite common) case where no DOM mutations occur during a Task. I'll look into this...
> > 
> > Having looked into it, it's not clear that this is viable (especially considering that it won't be able to be generalized for microtasks as well as tasks), so I'm going to move ahead with runEndOfTaskWork() for now.
> 
> Can you describe the problem further?  It seems like this is just about maintaining a queue of tasks to run after the current task completes.  Maybe it actually needs to be a stack of queues in order to handle the case of further microtasks generated during the processing of a microtask.

Or, if you wanted the queue to live in WebKit, then you could just make WebThread have a setEndOfTaskWork(Task*), and then say that there can only be one per task.  That seems a bit less clean to me, however, it is basically equivalent to WebKit::runEndOfTaskWork.

Have you thought about how micro-tasks should work on webworker threads?  Maybe we don't need them there :)

[Rafael Weinstein]

Sorry. To clarify:

Backing up a second here. I think there are three potentially orthogonal issues:

1) The end-of-microtask timing really means two things:
  a) At the end of the outer-most script invocation
  b) At the end of the task

Where (b) is kind of a catch all in case work is scheduled outside of a script invocation

2) When we find a second client of end-of-task work, we'll likely want to generalize deliverAllMutations() to be runMicroTasks(), of which MutationObservers and the new thing are clients.

3) At that point, the question arises: should WebCore own the microTask queue, or should the embedder.

Darin, it sounds to me like you're anticipating (2) and see value in (3) being the embedder.

Is this a fair summary of the issues? If so, can you help me (us) understand your thinking.

[Darin Fisher (:fishd, Google)]

> Darin, it sounds to me like you're anticipating (2) and see value in (3) being the embedder.
> 
> Is this a fair summary of the issues?

Yes, bingo!


> If so, can you help me (us) understand your thinking.

I think there are a number of other mutation or side-effect events generated from actions in WebCore that really should be processed using micro-tasks.  Good examples are resize and scroll event handlers.  We just made those fully asynchronous, but as a result we made them less useful.  They were made asynchronous to avoid re-entrancy problems.  I can readily imagine further use cases in the future for micro-tasks.  It is a nice idea!

[Adam Klein]

(In reply to comment #9)
> > Darin, it sounds to me like you're anticipating (2) and see value in (3) being the embedder.
> > 
> > Is this a fair summary of the issues?
> 
> Yes, bingo!
> 
> 
> > If so, can you help me (us) understand your thinking.
> 
> I think there are a number of other mutation or side-effect events generated from actions in WebCore that really should be processed using micro-tasks.  Good examples are resize and scroll event handlers.  We just made those fully asynchronous, but as a result we made them less useful.  They were made asynchronous to avoid re-entrancy problems.  I can readily imagine further use cases in the future for micro-tasks.  It is a nice idea!

I think we're all agreed that this new timing will be useful. The question in play is where that queue of things-to-do-at-the-end-of-microtask should live.  Right now, it lives in WebCore (managed by WebKitMutationObserver), and all the work is run when we leave the outermost script invocation (in V8Proxy::didLeaveScriptContext for ports that use V8).

There's no reason we couldn't generalize that code while leaving it inside WebCore. I.e., we could add WebCore::postEndOfMicroTaskWork() (which would be called by the mutation observation mechanism) and WebCore::runEndOfMicroTaskWork() (which would be called by both didLeaveScriptContext and WebKit::runEndOfTaskWork()).

If we were to move the queue out of WebCore, we'd still need some way for V8Proxy::didLeaveScriptContext to tell the embedder when a microtask ends, so it could do its work. And the benefit of having the queue owned by the embedder is unclear to us; did you have something in mind?

If your concern is simply that WebCore's interface to this end-of-microtask timing should be generalized, that's fine, but it's orthogonal to the question of how the end-of-_task_ work is triggered by the embedder (which is what this patch is trying to address).

Sorry this is tricky to discuss via email, the distinction between _task_ and _microtask_ is subtle, and the similarity of their names isn't helping.

[Darin Fisher (:fishd, Google)]

OK, I think I have a better understanding now.  I can see why having the queue live in WebCore would be simpler.  I don't have a good reason for putting the queue outside of WebCore.

I think that there is a fundamental primitive of the MessageLoop that we want to expose, which enables us to schedule work to follow the current task.  That seems like a slightly better primitive to expose.  Maybe this just amounts to adding a TaskObserver to WebThread (mirroring the one on Chromium's MessageLoop).

[Adam Klein]

(In reply to comment #11)
> OK, I think I have a better understanding now.  I can see why having the queue live in WebCore would be simpler.  I don't have a good reason for putting the queue outside of WebCore.
> 
> I think that there is a fundamental primitive of the MessageLoop that we want to expose, which enables us to schedule work to follow the current task.  That seems like a slightly better primitive to expose.  Maybe this just amounts to adding a TaskObserver to WebThread (mirroring the one on Chromium's MessageLoop).

That sounds good to me, I'll see about implementing either WebThread::postEndOfTaskWork or WebThread::addTaskObserver (I think the latter is likely to do the best job of letting the WebCore logic fully explain what's going on).

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> (In reply to comment #11)
> > OK, I think I have a better understanding now.  I can see why having the queue live in WebCore would be simpler.  I don't have a good reason for putting the queue outside of WebCore.
> > 
> > I think that there is a fundamental primitive of the MessageLoop that we want to expose, which enables us to schedule work to follow the current task.  That seems like a slightly better primitive to expose.  Maybe this just amounts to adding a TaskObserver to WebThread (mirroring the one on Chromium's MessageLoop).
> 
> That sounds good to me, I'll see about implementing either WebThread::postEndOfTaskWork or WebThread::addTaskObserver (I think the latter is likely to do the best job of letting the WebCore logic fully explain what's going on).

Agreed.  I'm anticipating a patch that introduces addTaskObserver :-)

[Adam Klein]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > OK, I think I have a better understanding now.  I can see why having the queue live in WebCore would be simpler.  I don't have a good reason for putting the queue outside of WebCore.
> > > 
> > > I think that there is a fundamental primitive of the MessageLoop that we want to expose, which enables us to schedule work to follow the current task.  That seems like a slightly better primitive to expose.  Maybe this just amounts to adding a TaskObserver to WebThread (mirroring the one on Chromium's MessageLoop).
> > 
> > That sounds good to me, I'll see about implementing either WebThread::postEndOfTaskWork or WebThread::addTaskObserver (I think the latter is likely to do the best job of letting the WebCore logic fully explain what's going on).
> 
> Agreed.  I'm anticipating a patch that introduces addTaskObserver :-)

So I've put together the addTaskObserver patch, but ran into a snag in Chromium: WebThreadImplForMessageLoop only has a MessageLoopProxy, not a MessageLoop, and it's not obviously simple (or sensical) to add a pass-through for AddTaskObserver to MessageLoopProxy.

How would you feel about something like WebKitPlatformSupport::addEventLoopObserver() (or addMainEventLoopObserver)?  This would allow me to skirt around the above hiccup, but would only allow clients to observe tasks on the main thread (fine for my purpose, as well as any other DOM-based APIs).  It's be sort of analogous to callOnMainThread.

[Darin Fisher (:fishd, Google)]

(In reply to comment #14)
> So I've put together the addTaskObserver patch, but ran into a snag in Chromium: WebThreadImplForMessageLoop only has a MessageLoopProxy, not a MessageLoop, and it's not obviously simple (or sensical) to add a pass-through for AddTaskObserver to MessageLoopProxy.
> 
> How would you feel about something like WebKitPlatformSupport::addEventLoopObserver() (or addMainEventLoopObserver)?  This would allow me to skirt around the above hiccup, but would only allow clients to observe tasks on the main thread (fine for my purpose, as well as any other DOM-based APIs).  It's be sort of analogous to callOnMainThread.

Well, since AddTaskObserver can only be called on the thread that owns the MessageLoop, you don't need to use MessageLoopProxy.  Just use MessageLoop::current()->AddTaskObserver().  You should use MessageLoopProxy::BelongsToCurrentThread to verify that addTaskObserver is being called on the correct thread.

[Adam Klein]

Created attachment 115657 [details]
WebTaskObserver

[Darin Fisher (:fishd, Google)]

Comment on attachment 115657 [details]
WebTaskObserver

View in context: https://bugs.webkit.org/attachment.cgi?id=115657&action=review

> Source/WebKit/chromium/public/WebTaskObserver.h:43
> +    virtual void willProcessTask() = 0;

Do you need willProcessTask?  Or, is this just for future-proofness?

[Adam Klein]

Comment on attachment 115657 [details]
WebTaskObserver

View in context: https://bugs.webkit.org/attachment.cgi?id=115657&action=review

>> Source/WebKit/chromium/public/WebTaskObserver.h:43
>> +    virtual void willProcessTask() = 0;
> 
> Do you need willProcessTask?  Or, is this just for future-proofness?

Future-proofness, happy to remove it if you'd rather keep this as minimal as possible (I'm already not matching MessageLoop::TaskObserver since I've removed the TimeTicks).

[Darin Fisher (:fishd, Google)]

(In reply to comment #18)
> (From update of attachment 115657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115657&action=review
> 
> >> Source/WebKit/chromium/public/WebTaskObserver.h:43
> >> +    virtual void willProcessTask() = 0;
> > 
> > Do you need willProcessTask?  Or, is this just for future-proofness?
> 
> Future-proofness, happy to remove it if you'd rather keep this as minimal as possible (I'm already not matching MessageLoop::TaskObserver since I've removed the TimeTicks).

I'd probably remove it since we don't have a need for it.  Also, it seems you decided not to extend WebThread.  I'm curious about that decision..

[Adam Klein]

Comment on attachment 115657 [details]
WebTaskObserver

View in context: https://bugs.webkit.org/attachment.cgi?id=115657&action=review

>>>> Source/WebKit/chromium/public/WebTaskObserver.h:43
>>>> +    virtual void willProcessTask() = 0;
>>> 
>>> Do you need willProcessTask?  Or, is this just for future-proofness?
>> 
>> Future-proofness, happy to remove it if you'd rather keep this as minimal as possible (I'm already not matching MessageLoop::TaskObserver since I've removed the TimeTicks).
> 
> I'd probably remove it since we don't have a need for it.  Also, it seems you decided not to extend WebThread.  I'm curious about that decision..

I guess my previous comment about MessageLoopProxy was unclear: by extending WebThread, I'm assuming you wanted something like the following to work:

webKitPlatformSupport->currentThread()->addTaskObserver(observer);

But currentThread() returns a WebThreadImplForMessageLoop, which wraps a MessageLoopProxy, and as previously-mentioned doesn't have an AddTaskObserver method (and likely shouldn't).

For my purposes, I'd be perfectly happy if the WebThread returned by currentThread() wasn't persistent, but the other user, CCThreadImpl, holds onto a reference, and for that use case it seems safer to have a MessageLoopProxy under the covers.

If you had a different API in mind than the code above, please explain.

[Darin Fisher (:fishd, Google)]

Yes, I understood :-)

This:

  webKitPlatformSupport->currentThread()->addTaskObserver(observer);

Can be implemented like this:

  DCHECK(message_loop_proxy_->BelongsToCurrentThread());
  MessageLoop::current()->AddTaskObserver(new TaskObserverAdaptor(observer));

[Adam Klein]

(In reply to comment #21)
> Yes, I understood :-)
> 
> This:
> 
>   webKitPlatformSupport->currentThread()->addTaskObserver(observer);
> 
> Can be implemented like this:
> 
>   DCHECK(message_loop_proxy_->BelongsToCurrentThread());
>   MessageLoop::current()->AddTaskObserver(new TaskObserverAdaptor(observer));

Ah, I see...that seems a bit weird to me (since it's only guarded by a DCHECK), but it does quite clearly mirror the Chromium interface, and thus retains the mapping MessageLoop <-> WebThread.  Not sure why I was being dense, I guess if I was designing MessageLoop::AddTaskObserver I would've made it a static that only operates on the current loop :)

[Darin Fisher (:fishd, Google)]

(In reply to comment #22)
> (In reply to comment #21)
> > Yes, I understood :-)
> > 
> > This:
> > 
> >   webKitPlatformSupport->currentThread()->addTaskObserver(observer);
> > 
> > Can be implemented like this:
> > 
> >   DCHECK(message_loop_proxy_->BelongsToCurrentThread());
> >   MessageLoop::current()->AddTaskObserver(new TaskObserverAdaptor(observer));
> 
> Ah, I see...that seems a bit weird to me (since it's only guarded by a DCHECK), but it does quite clearly mirror the Chromium interface, and thus retains the mapping MessageLoop <-> WebThread.  Not sure why I was being dense, I guess if I was designing MessageLoop::AddTaskObserver I would've made it a static that only operates on the current loop :)

^^^ good point!  it probably would be better as a static.  you could change that DCHECK to a CHECK by the way.

[Adam Klein]

Created attachment 116815 [details]
Moved to WebThread

[Adam Klein]

Created attachment 117032 [details]
Patch for landing

[Adam Klein]

Created attachment 117034 [details]
Patch for landing

[Adam Klein]

Committed r101418: <http://trac.webkit.org/changeset/101418>

[Adam Klein]

(In reply to comment #27)
> Committed r101418: <http://trac.webkit.org/changeset/101418>

This causes Chromium renderers to fail to start, due to webKitPlatformSupport->currentThread() returning NULL.  If I can't figure this out quickly I'll rollout (https://bugs.webkit.org/show_bug.cgi?id=73372)

[Adam Klein]

(In reply to comment #28)
> (In reply to comment #27)
> > Committed r101418: <http://trac.webkit.org/changeset/101418>
> 
> This causes Chromium renderers to fail to start, due to webKitPlatformSupport->currentThread() returning NULL.  If I can't figure this out quickly I'll rollout (https://bugs.webkit.org/show_bug.cgi?id=73372)

Okay, I've found the problem. It has to do with Chromium/Linux's SandboxIPCProcess (http://code.google.com/p/chromium/wiki/LinuxSandboxIPC), which calls WebKit::initialize() without providing a MessageLoop.

Two possible fixes include:
- Checking to see whether webKitPlatformSupport->currentThread() is non-null before operating on it.
- Calling WebKit::initializeWithoutV8() in the sandbox process, and thus avoiding our code.

The latter seems to work, but I'm not clear on exactly what the process needs from WebKit. The former more obviously works, but feels weird (why would currentThread() be null?).  Suggestions appreciated.

[Adam Klein]

+agl, who may be able to answer what sort of WebKit initialization is required by the SandboxIPCProcess.

[Adam Klein]

+levin, who added the WebKit::initialize() call in http://crrev.com/95533.

It looks like this call to initialize() is unnecessary for proper operation.  Perhaps it can just be removed?  Looking through WebKit commit logs, I don't see the change Dave refers to when he says "Starting DumpRenderTree will assert without this after I land a change in WebKIt." [sic].

[Adam Klein]

Created attachment 117283 [details]
For re-landing

[Adam Klein]

Created attachment 117658 [details]
Patch for landing

[Adam Klein]

Committed r101842: <http://trac.webkit.org/changeset/101842>