Bug 226734 – Drop legacy MainThreadTaskQueue & EventLoopTaskQueue classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 430757 [details]
Patch

[Chris Dumez]

Created attachment 430771 [details]
Patch

[Chris Dumez]

Created attachment 430772 [details]
Patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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).

[Chris Dumez]

(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.

[Chris Dumez]

(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 :)

[Ryosuke Niwa]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 430788 [details]
Patch

[Chris Dumez]

Created attachment 430789 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/78973554>