Bug 137496 – Add a microtask abstraction

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Add a microtask abstraction

[Yoav Weiss]

Created attachment 239429 [details]
Patch

[Yoav Weiss]

I've split out the MicroTask part from https://bugs.webkit.org/show_bug.cgi?id=134488 into this separate bug & patch.

Since the MicroTask interface doesn't have users/implementors yet, this patch is not testable on its own. Tests will be added with the async image loading patch.

[Yoav Weiss]

Created attachment 239430 [details]
Patch

[Darin Adler]

Comment on attachment 239430 [details]
Patch

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

> Source/WebCore/dom/MicroTask.h:28
> +class MicroTask : public RefCounted<MicroTask> {

Why do these need to be reference counted? Can we use std::unique_ptr instead and use single ownership?

> Source/WebCore/dom/MicroTaskQueue.h:31
> +class MicroTaskQueue : public RefCounted<MicroTaskQueue> {

I don’t think we should use a reference-counted object just to hold a Vector. We should simply have the vector of tasks in Document itself. This level of indirection doesn’t help us. No need to have a class for this.

> Source/WebCore/dom/MicroTaskQueue.h:42
> +        for (auto task : m_tasks)
> +            task->run();
> +        m_tasks.clear();

What prevents this from being reentered, running the same task more than once?

[Darin Adler]

Comment on attachment 239430 [details]
Patch

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

> Source/WebCore/dom/ScriptRunner.cpp:121
> +    m_document.runMicroTasks();

What guarantees we aren’t already executing a script here?

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:236
> +    m_document->runMicroTasks();

What guarantees we aren’t already inside a script here?

[Yoav Weiss]

Created attachment 239604 [details]
Patch

[Yoav Weiss]

(In reply to comment #4)
> (From update of attachment 239430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239430&action=review
> 
> > Source/WebCore/dom/MicroTask.h:28
> > +class MicroTask : public RefCounted<MicroTask> {
> 
> Why do these need to be reference counted? Can we use std::unique_ptr instead and use single ownership?

You're right, they don't need to be. I removed it.

> 
> > Source/WebCore/dom/MicroTaskQueue.h:31
> > +class MicroTaskQueue : public RefCounted<MicroTaskQueue> {
> 
> I don’t think we should use a reference-counted object just to hold a Vector. We should simply have the vector of tasks in Document itself. This level of indirection doesn’t help us. No need to have a class for this.

Removed.

> 
> > Source/WebCore/dom/MicroTaskQueue.h:42
> > +        for (auto task : m_tasks)
> > +            task->run();
> > +        m_tasks.clear();
> 
> What prevents this from being reentered, running the same task more than once?

I assumed the microtask queue would be accessed only from the main thread. I've added ASSERTs to make sure this assumption stands.

(In reply to comment #5)
> (From update of attachment 239430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239430&action=review
> 
> > Source/WebCore/dom/ScriptRunner.cpp:121
> > +    m_document.runMicroTasks();
> 
> What guarantees we aren’t already executing a script here?

I've interpreted that code as the code running HTMLScriptElement's code one by one in the loop above my code addition. Therefore, I didn't think such protection is necessary.
It's highly probable that I was wrong about that though. If that's the case, I'd appreciate guidance at to how I can add the required protection here.

> 
> > Source/WebCore/html/parser/HTMLScriptRunner.cpp:236
> > +    m_document->runMicroTasks();
> 
> What guarantees we aren’t already inside a script here?

I've now protected against this. Thanks for catching that.

[Yoav Weiss]

ping

[Myles C. Maxfield]

Comment on attachment 239604 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests for this patch. Tests will be added in a followup patch that uses it.

:(

[Yoav Weiss]

Yeah, I'm not thrilled about adding unused and untestable code either, but I was told that it's better to split out the microtask portion from https://bugs.webkit.org/show_bug.cgi?id=134488 that includes usage (and will include tests).

[Yoav Weiss]

Ping. This is still waiting for a review. Thanks :)

[Sam Weinig]

Microtasks are defined as being per Event Loop (https://html.spec.whatwg.org/multipage/webappapis.html#event-loops), which is not quite the same as per-document.  I believe this would be observable, as microtasks in multiple documents would be run out of phase.

I also think this would be testable by moving Mutation Observers over to the new abstraction as you build it. Or, if you prefer a less invasive change at the same time, you could add an Internal function that queues a microtask, and use that for testing.

[Yoav Weiss]

Thanks for the review! :)

> Microtasks are defined as being per Event Loop 
> (https://html.spec.whatwg.org/multipage/webappapis.html#event-loops), which is
> not quite the same as per-document.  I believe this would be observable, as
> microtasks in multiple documents would be run out of phase.

What would be a better place from which to call runMicroTasks so that the microtasks of different documents would be in sync?

[Sam Weinig]

(In reply to comment #13)
> Thanks for the review! :)
> 
> > Microtasks are defined as being per Event Loop 
> > (https://html.spec.whatwg.org/multipage/webappapis.html#event-loops), which is
> > not quite the same as per-document.  I believe this would be observable, as
> > microtasks in multiple documents would be run out of phase.
> 
> What would be a better place from which to call runMicroTasks so that the
> microtasks of different documents would be in sync?

It would either need to be global (which I would rather it not be) or on a new object that is shared between Documents that need to be synchronized. As the spec says, the documents that need to be synchronized are the ones that need to share an Event Loop of which, "There must be at least one browsing context event loop per user agent, and at most one per unit of related similar-origin browsing contexts".

[Yoav Weiss]

Is there an object that is common to all Documents that need to be synchronized, which can own the MicroTask Queue? I'd like to avoid a global/singleton as well.

[Sam Weinig]

(In reply to comment #15)
> Is there an object that is common to all Documents that need to be
> synchronized, which can own the MicroTask Queue? I'd like to avoid a
> global/singleton as well.

There is not one currently.

[Yoav Weiss]

Going back to this, I wonder what's the best way to avoid a global here.
If I understand correctly, we have a single event loop per renderer process. We want microtasks running on this event loop to be synchronized (e.g. pages from a similar origin must be on the same event loop, and therefore should have their microtasks running sequentially).

Looking around the code, it seems like the one-to-one relationship with Document goes all the way up to WebPage. IIUC, WebPages are then owned by WebKit2's WebProcess, which makes me think that WebProcess is the right place to maintain such an object that would represent EventLoop synchronization. Is that correct?
If so, all the WebPages owned by a single WebProcess share the same event loop, right?

[Yoav Weiss]

After adding the microtask queue on WebProcess, I realized that it'd mean I need to add it on Source/WebKit as well, which seemed messy.
I'm now leaning towards adding it as a singleton. Please let me know if this is not an acceptable way forward.

[Sam Weinig]

(In reply to comment #18)
> After adding the microtask queue on WebProcess, I realized that it'd mean I
> need to add it on Source/WebKit as well, which seemed messy.
> I'm now leaning towards adding it as a singleton. Please let me know if this
> is not an acceptable way forward.

After trying to think this through, I think a singleton is probably the best way forward for now.

[Yoav Weiss]

Created attachment 246886 [details]
Patch

[Yoav Weiss]

I've added a singleton object as well as testing through an internals method.

[Yoav Weiss]

Created attachment 246888 [details]
Patch

[Yoav Weiss]

Created attachment 246889 [details]
Patch

[Yoav Weiss]

Created attachment 246922 [details]
Patch

[Yoav Weiss]

Created attachment 247006 [details]
Patch

[Yoav Weiss]

Created attachment 247083 [details]
Patch

[Yoav Weiss]

And now it's all green! Sam, can you please take another look?

[Sam Weinig]

Comment on attachment 247083 [details]
Patch

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

What happens if a microtask for one document is still enqued when that document goes away or into the back/forward cache? A great follow up would be move MutationObservers to be built on this.

> Source/WebCore/testing/MicroTaskTest.h:31
> +class MicroTaskTest: public MicroTask {

Should have a space after MicroTaskTest.

[Yoav Weiss]

Created attachment 247667 [details]
Patch

[Yoav Weiss]

> What happens if a microtask for one document is still enqued when that document goes away or into the back/forward cache?

Good catch. I've changed the test MicroTask to only hold a weak pointer to the document, and added a test to make sure we handle that case well.

> Should have a space after MicroTaskTest.

Added.

[Yoav Weiss]

Created attachment 247669 [details]
Patch

[Yoav Weiss]

Created attachment 247697 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 247697 [details]
Patch

Clearing flags on attachment: 247697

Committed r180911: <http://trac.webkit.org/changeset/180911>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142204

[Mark Rowe (bdash)]

class MicroTask : public RefCounted<MicroTask> {
…
   WEBCORE_EXPORT void queueMicroTask(std::unique_ptr<MicroTask>);

Having unique_ptrs to a RefCounted object seems incorrect. It's likely the reason for the assertion failures that led to me rolling this out.

[Yoav Weiss]

Created attachment 247752 [details]
Patch

[Yoav Weiss]

I've fixed it, ran the tests locally with a debug build and now they're running fine.

[Yoav Weiss]

Created attachment 247812 [details]
Patch

[Yoav Weiss]

Sam, can you please re-review?

[Sam Weinig]

Comment on attachment 247812 [details]
Patch

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

> Source/WebCore/dom/MicroTask.cpp:26
> +#include <wtf/text/StringBuilder.h>

StringBuilder.h is unused in this file.  You may however need <wtf/NeverDestroyed.h>

[Yoav Weiss]

Created attachment 247843 [details]
Patch

[Yoav Weiss]

> StringBuilder.h is unused in this file.  You may however need <wtf/NeverDestroyed.h>
[reply] [-] Comment 42

Removed StringBuilder. NeverDestroyed is included in the h file, which we include here.

[WebKit Commit Bot]

Comment on attachment 247843 [details]
Patch

Clearing flags on attachment: 247843

Committed r180996: <http://trac.webkit.org/changeset/180996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.