Summary: | Add a microtask abstraction | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yoav Weiss <yoav> | ||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, dino, esprehn+autocc, gyuyoung.kim, kangil.han, rniwa, sam, simon.fraser | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | 142204 | ||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yoav Weiss
2014-10-07 12:49:16 PDT
Created attachment 239429 [details]
Patch
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. Created attachment 239430 [details]
Patch
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? 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? Created attachment 239604 [details]
Patch
(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. ping 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. :( 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). Ping. This is still waiting for a review. Thanks :) 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. 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?
(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". 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. (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. 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? 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. (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. Created attachment 246886 [details]
Patch
I've added a singleton object as well as testing through an internals method. Created attachment 246888 [details]
Patch
Created attachment 246889 [details]
Patch
Created attachment 246922 [details]
Patch
Created attachment 247006 [details]
Patch
Created attachment 247083 [details]
Patch
And now it's all green! Sam, can you please take another look? 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. Created attachment 247667 [details]
Patch
> 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. Created attachment 247669 [details]
Patch
Created attachment 247697 [details]
Patch
Comment on attachment 247697 [details] Patch Clearing flags on attachment: 247697 Committed r180911: <http://trac.webkit.org/changeset/180911> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 142204 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. Created attachment 247752 [details]
Patch
I've fixed it, ran the tests locally with a debug build and now they're running fine. Created attachment 247812 [details]
Patch
Sam, can you please re-review? 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> Created attachment 247843 [details]
Patch
> 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. Comment on attachment 247843 [details] Patch Clearing flags on attachment: 247843 Committed r180996: <http://trac.webkit.org/changeset/180996> All reviewed patches have been landed. Closing bug. |