1350953 - Label service worker runnables

Status: RESOLVED WONTFIX

(Core :: DOM: Service Workers, enhancement, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

See Bug 1321812
	
I have no idea how easy it is to get reasonable Tab/DocGroup to dispatch.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

As I know, WorkerMainThreadRunnable and WorkerPrivate::DispatchToMainThread() use EventTargetFor(TaskCategory::Worker) for main-thread dispatching:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/workers/WorkerPrivate.cpp#4479

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Note, most of the ServiceWorkerManager and its associated runnables will be moving to parent process or worker process in the near future.  Labeling the current runnables may not be worth prioritizing at the moment.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Also, I anticipate that we may start using MozPromise more in service worker code.  I'm not sure how that impacts labeling efforts.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

MozPromise uses still normal event loop somewhere underneath, so they should be labeled.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

But if all that serviceworker stuff in content processes will just go away, nothing to label.
(New code of course should be labeled)

Comment 6

[Mahendra Potharaju [:mahe]]

Olli, Do we need a new bug to track the labeling of the new code? Or will labeling be implemented as part of new development?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I think bevis is better person to ask labeling questions.
But in general I'd prefer if all the new code was labeled properly from the beginning.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Yes, labeling shall be part of new development.
For the use of MozPromise, it's more related to which AbstractThread instance shall be used.
For MozPromises on the main thread, SchedulerGroup::AbstractMainThreadFor() is the option for labeling:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/xpcom/threads/SchedulerGroup.h#74

For the use of MozPromise off the main thread,
it's still TBD to get the default AbstractThread instance.
AFAIK, AbstractThread::GetCurrent() is the current option but it's a wrong option and is abused in several places.

In short, current implementation of AbstractThread::GetCurrent() is wrong.
The default TLS value of AbstractThread::sCurrentThreadTLS is set to AbstractThread::MainThread() on main thread and set to MessageLoopAbstractThreadWrapper off main thread which is incorrect!

According to the comment in its declaration,
The return value of AbstractThread::GetCurrent() shall always be nullptr unless the caller is in a runnable that was dispatched to an associated AbstractThread:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/xpcom/threads/AbstractThread.h#40-42

:jwwang might have better suggestion of how AbstractThread shall be accessed by default in the future for MozPromise.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I think having different APIs for main thread vs other code makes it harder to build features that need to work on both window and worker.  Shouldn't we be striving to have less "if main thread do special logic" stuff in the tree?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> :jwwang might have better suggestion of how AbstractThread shall be accessed
> by default in the future for MozPromise.

I am not sure I follow. MozPromise never assumes a particular AbstractThread to use. It is up to the caller to supply a target thread to MozPromise::Then(...).

Comment 11

[Jens Stutte [:jstutte]]

:smaug, this blocks an already resolved issue. Can I then assume, we do not need/want this any more?

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jens Stutte [:jstutte] from comment #11)

:smaug, this blocks an already resolved issue. Can I then assume, we do not need/want this any more?

Yes.