Bug 226734 - Drop legacy MainThreadTaskQueue & EventLoopTaskQueue classes
Summary: Drop legacy MainThreadTaskQueue & EventLoopTaskQueue classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-07 09:22 PDT by Chris Dumez
Modified: 2021-06-07 17:30 PDT (History)
22 users (show)

See Also:


Attachments
Patch (52.59 KB, patch)
2021-06-07 09:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (52.80 KB, patch)
2021-06-07 12:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (52.70 KB, patch)
2021-06-07 12:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (53.47 KB, patch)
2021-06-07 15:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (52.42 KB, patch)
2021-06-07 15:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-06-07 09:22:45 PDT
Drop legacy MainThreadTaskQueue & EventLoopTaskQueue classes. Code that was using MainThreadTaskQueue is now calling callOnMainThread() directly. Call that was using EventLoopTaskQueue is now using the HTML event loop directly. If said code needed to cancel tasks or check if a previously scheduled task is still pending, it now relies on WTF::CancellableTask / WTF::TaskCanceller to do so.
Comment 1 Chris Dumez 2021-06-07 09:46:56 PDT
Created attachment 430757 [details]
Patch
Comment 2 Chris Dumez 2021-06-07 12:15:12 PDT
Created attachment 430771 [details]
Patch
Comment 3 Chris Dumez 2021-06-07 12:16:32 PDT
Created attachment 430772 [details]
Patch
Comment 4 Ryosuke Niwa 2021-06-07 14:57:27 PDT
Comment on attachment 430772 [details]
Patch

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

> Source/WTF/wtf/CancellableTask.h:36
> +class TaskCanceller : public CanMakeWeakPtr<TaskCanceller> {

I would have called this TaskCancelGroup or something.

> Source/WTF/wtf/CancellableTask.h:54
> +        void taskIsNoLongerPending() { m_impl = nullptr; }

This should be a verb instead.
maybe clear?

> Source/WebCore/dom/ActiveDOMObject.h:118
> +        CancellableTask cancellableTask(taskCanceller, WTFMove(task));
>          object.queueTaskInEventLoop(source, [protectedObject = makeRef(object), activity = object.ActiveDOMObject::makePendingActivity(object), cancellableTask = WTFMove(cancellableTask)]() mutable {

Hm... this would mean that the object is kept alive with a pending activity even when the task is canceled?
That doesn't seem ideal.
We should probably follow up so that we can delete the task object itself from the event loop queue when we cancel a task.
Comment 5 Ryosuke Niwa 2021-06-07 14:59:47 PDT
By the way, we need to figure out a way to replace callOnMainThread with some kind of EventLoop equivalent as well because for the purpose of implementing request idle callback, we need to be aware of these things as well. And without origin attribution, we won't be able to tell whether a given task should delay the idle callback or not (delaying for a cross-origin would be a cross-origin information leak).
Comment 6 Chris Dumez 2021-06-07 15:02:59 PDT
(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 430772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430772&action=review
> 
> > Source/WTF/wtf/CancellableTask.h:36
> > +class TaskCanceller : public CanMakeWeakPtr<TaskCanceller> {
> 
> I would have called this TaskCancelGroup or something.

Yes, let me think about more about the naming. I agree that TaskCanceller is not great but I'm not a big fan of TaskCancelGroup either.
Maybe CancellableTaskGroup?

> 
> > Source/WTF/wtf/CancellableTask.h:54
> > +        void taskIsNoLongerPending() { m_impl = nullptr; }
> 
> This should be a verb instead.
> maybe clear?

I had clear() in a previous iteration. I'll go back to that.

> 
> > Source/WebCore/dom/ActiveDOMObject.h:118
> > +        CancellableTask cancellableTask(taskCanceller, WTFMove(task));
> >          object.queueTaskInEventLoop(source, [protectedObject = makeRef(object), activity = object.ActiveDOMObject::makePendingActivity(object), cancellableTask = WTFMove(cancellableTask)]() mutable {
> 
> Hm... this would mean that the object is kept alive with a pending activity
> even when the task is canceled?
> That doesn't seem ideal.
> We should probably follow up so that we can delete the task object itself
> from the event loop queue when we cancel a task.

At worst, it delays the the deletion until the next event loop iteration though, no? It didn't sound terrible to me but maybe I am missing something.
Comment 7 Chris Dumez 2021-06-07 15:04:04 PDT
(In reply to Ryosuke Niwa from comment #5)
> By the way, we need to figure out a way to replace callOnMainThread with
> some kind of EventLoop equivalent as well because for the purpose of
> implementing request idle callback, we need to be aware of these things as
> well. And without origin attribution, we won't be able to tell whether a
> given task should delay the idle callback or not (delaying for a
> cross-origin would be a cross-origin information leak).

I see, that's good to know. Baby steps in the right direction still :)
Comment 8 Ryosuke Niwa 2021-06-07 15:08:02 PDT
(In reply to Chris Dumez from comment #6)
> (In reply to Ryosuke Niwa from comment #4)
> > Comment on attachment 430772 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=430772&action=review
> > 
> > > Source/WTF/wtf/CancellableTask.h:36
> > > +class TaskCanceller : public CanMakeWeakPtr<TaskCanceller> {
> > 
> > I would have called this TaskCancelGroup or something.
> 
> Yes, let me think about more about the naming. I agree that TaskCanceller is
> not great but I'm not a big fan of TaskCancelGroup either.
> Maybe CancellableTaskGroup?

Yeah, CancellableTaskGroup sounds good although this sounds like a generic grouping of cancelable tasks. "cancel-group" communicates that it's a unit of cancellation. Maybe TaskCancellationGroup?

> > > Source/WebCore/dom/ActiveDOMObject.h:118
> > > +        CancellableTask cancellableTask(taskCanceller, WTFMove(task));
> > >          object.queueTaskInEventLoop(source, [protectedObject = makeRef(object), activity = object.ActiveDOMObject::makePendingActivity(object), cancellableTask = WTFMove(cancellableTask)]() mutable {
> > 
> > Hm... this would mean that the object is kept alive with a pending activity
> > even when the task is canceled?
> > That doesn't seem ideal.
> > We should probably follow up so that we can delete the task object itself
> > from the event loop queue when we cancel a task.
> 
> At worst, it delays the the deletion until the next event loop iteration
> though, no? It didn't sound terrible to me but maybe I am missing something.

Yeah but the event loop iteration could be many hundreds of milliseconds away. This would mean that any temporary object we create will be effectively leaked until the current task finishes. That's not great.
Comment 9 Chris Dumez 2021-06-07 15:12:23 PDT
(In reply to Ryosuke Niwa from comment #8)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Ryosuke Niwa from comment #4)
> > > Comment on attachment 430772 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=430772&action=review
> > > 
> > > > Source/WTF/wtf/CancellableTask.h:36
> > > > +class TaskCanceller : public CanMakeWeakPtr<TaskCanceller> {
> > > 
> > > I would have called this TaskCancelGroup or something.
> > 
> > Yes, let me think about more about the naming. I agree that TaskCanceller is
> > not great but I'm not a big fan of TaskCancelGroup either.
> > Maybe CancellableTaskGroup?
> 
> Yeah, CancellableTaskGroup sounds good although this sounds like a generic
> grouping of cancelable tasks. "cancel-group" communicates that it's a unit
> of cancellation. Maybe TaskCancellationGroup?

TaskCancellationGroup sounds good to me.

> 
> > > > Source/WebCore/dom/ActiveDOMObject.h:118
> > > > +        CancellableTask cancellableTask(taskCanceller, WTFMove(task));
> > > >          object.queueTaskInEventLoop(source, [protectedObject = makeRef(object), activity = object.ActiveDOMObject::makePendingActivity(object), cancellableTask = WTFMove(cancellableTask)]() mutable {
> > > 
> > > Hm... this would mean that the object is kept alive with a pending activity
> > > even when the task is canceled?
> > > That doesn't seem ideal.
> > > We should probably follow up so that we can delete the task object itself
> > > from the event loop queue when we cancel a task.
> > 
> > At worst, it delays the the deletion until the next event loop iteration
> > though, no? It didn't sound terrible to me but maybe I am missing something.
> 
> Yeah but the event loop iteration could be many hundreds of milliseconds
> away. This would mean that any temporary object we create will be
> effectively leaked until the current task finishes. That's not great.

Ok, I'll see if I can do better in a follow-up.
Comment 10 Chris Dumez 2021-06-07 15:23:46 PDT
Created attachment 430788 [details]
Patch
Comment 11 Chris Dumez 2021-06-07 15:28:32 PDT
Created attachment 430789 [details]
Patch
Comment 12 EWS 2021-06-07 17:29:57 PDT
Committed r278580 (238574@main): <https://commits.webkit.org/238574@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430789 [details].
Comment 13 Radar WebKit Bug Importer 2021-06-07 17:30:26 PDT
<rdar://problem/78973554>