Bug 226734

Created attachment 430757 [details] Patch

Created attachment 430771 [details] Patch

Created attachment 430772 [details] Patch

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.

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

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

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

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

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

Created attachment 430788 [details] Patch

Created attachment 430789 [details] Patch

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

<rdar://problem/78973554>