1839103 - Can Task be marked to be freed only on main thread?

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Tooru Fujisawa [:arai]]

Currently there's no guarantee about in which thread the TaskController's Task is freed,
this includes a Task with mMainThreadOnly == true.

This means such Task cannot have RefPtr/nsCOMPtr that points main-thread-only object,
and it requires separate storage to track those pointers (similar thing is done for Runnable in NotifyOffThreadScriptCompletedRunnable).

If a Task with mMainThreadOnly == true (or with extra flag) can be guaranteed to be freed only on main thread, the pointer handling will become easier.

Comment 1

[Tooru Fujisawa [:arai]]

:Bas, can I have your input here?
is it feasible to make Task freed only on main thread depending on flag or something?
or such RefPtr/nsCOMPtr should be stored into different main-thread-only object?

Comment 2

[Bas Schouten (:bas.schouten)]

(In reply to Tooru Fujisawa [:arai] from comment #1)

:Bas, can I have your input here?
is it feasible to make Task freed only on main thread depending on flag or something?
or such RefPtr/nsCOMPtr should be stored into different main-thread-only object?

We can certainly do that. Although I'd like to understand what the scenario is in which this -doesn't- happen? How would the last reference ever get released off the main thread?

As an alternative we could have a MainThreadReleaseOnlyTask class that you could inherit from that guarantees this. or something along those lines. (We can simply implement this by overloading the release function or something like that).

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Bas Schouten (:bas.schouten) from comment #2)

Although I'd like to understand what the scenario is in which this -doesn't- happen? How would the last reference ever get released off the main thread?

I haven't spotted such scenario.
So, if it's already true that a Task with mMainThreadOnly == true is freed only on main thread (including failure case),
and keeping such behavior doesn't block any other feature, we can add a comment and with some assertion to guarantee that.

But yeah, a dedicate subclass might be easier to understand and reason about when a task can have such pointer and when it cannot.

Comment 4

[Tooru Fujisawa [:arai]]

Okay, I've spotted one case.

RefPtr<Task_MainThreadOnly_True> A = MakeAndAddRef<Task_MainThreadOnly_True>();
RefPtr<Task_MainThreadOnly_False> B = MakeAndAddRef<Task_MainThreadOnly_False>();

B->AddDependency(A.get());

TaskController::Get()->AddTask(A.forget());
TaskController::Get()->AddTask(B.forget());

If a task A with mMainThreadOnly==true is added to dependency of other task B,
the task B's mDependencies becomes the last reference to A, and the reference gets released when the task B is about to be executed.
and if the task B has mMainThreadOnly==false and is executed off main thread, the task A's destructor is executed off main thread.

https://searchfox.org/mozilla-central/rev/d307d4d9f06dab6d16e963a4318e5e8ff4899141/xpcom/threads/TaskController.cpp#169,181

Task* Task::GetHighestPriorityDependency() {
...
        currentTask->mDependencies.erase(oldIter);

https://searchfox.org/mozilla-central/rev/d307d4d9f06dab6d16e963a4318e5e8ff4899141/xpcom/threads/TaskController.cpp#296,330

void TaskController::RunPoolThread() {
...
        while ((nextTask = task->GetHighestPriorityDependency())) {

Comment 5

[Tooru Fujisawa [:arai]]

So, as long as there's no pointer from off-thread task or any other off-thread thing, a main-thread-only task is freed on main thread.
I'll use that condition in bug 1845668 and bug 1839108.

Comment 6

[Tooru Fujisawa [:arai]]

I'll add assertion to Task::AddDependency to avoid adding depdendency from off-thread task to main-thread-only task.
if the assertion fails, then we need to have another flag for main-thread-only task whether the task can be freed off-main-thread or not.
both ScriptLoader and XUL case will need to be freed only on main thread.

Comment 7

[Tooru Fujisawa [:arai]]

no longer blocks bug 1839108