1363474 - SchedulerEventTarget::IsOnCurrentThread should map to the group itself rather than NS_IsMainThread

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

[Andrew Osmond [:aosmond] (he/him)]

There is currently no method to determine if the current execution context matches a given scheduler group -- SchedulerEventTarget::IsOnCurrentThread just defers to NS_IsMainThread. This is useful for consumers such as imagelib, which sometimes needs to dispatch the same notifications to consumers in different doc/tab/etc scheduler groups.

Ideally SchedulerEventTarget::IsOnCurrentThread would return true if and only if our currect execution context is equivalent and may run the same code that could otherwise be dispatched to that target. So, if the event target is doc group A / tab group A, and we are dispatched on tab group A, it would return true, because we can safely run code for any doc in our tab group. Similarly if we are dispatched via NS_DispatchToMainThread, it will also return true. But if we are in the system group, then it would return false.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This patch sort of does what you want. It sounds like you also want SchedulerEventTarget::IsOnCurrentThread to return true if we're running an unlabeled runnable and we're on the main thread. That doesn't seem like the contract that I would expect this method to have, so I didn't implement it that way. If you really need that functionality, maybe we could implement it in some other way.

However, my hope is that the worst thing that happens if this returns false is that you have to do an extra dispatch. If that's true, and if we succeed in labeling most/all of the runnables that need to dispatch imglib events, then maybe it doesn't matter what it does if an unlabeled runnable is running?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8866565 [details] [diff] [review]
patch

This is causing a bunch of assertions to fail. I'll need to figure out what to do.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Bill McCloskey (:billm) from comment #1)
> Created attachment 8866565 [details] [diff] [review]
> patch
> 
> This patch sort of does what you want. It sounds like you also want
> SchedulerEventTarget::IsOnCurrentThread to return true if we're running an
> unlabeled runnable and we're on the main thread. That doesn't seem like the
> contract that I would expect this method to have, so I didn't implement it
> that way. If you really need that functionality, maybe we could implement it
> in some other way.
> 
> However, my hope is that the worst thing that happens if this returns false
> is that you have to do an extra dispatch. If that's true, and if we succeed
> in labeling most/all of the runnables that need to dispatch imglib events,
> then maybe it doesn't matter what it does if an unlabeled runnable is
> running?

The above should be fine. I don't have a strong preference and most of the time we should not hit that condition. The extra dispatch and associated latency is the only reason I thought of it. If it becomes problematic, we can deal with it then.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch, v2

This is a simpler patch that adds a new method. It actually does what was asked for in comment 0: returns true when we're running an unlabeled runnable.

The reason the previous patch didn't work is that a ton of code does the following:

- Takes an event target as an argument, which it will dispatch to later
- Asserts aEventTarget->IsOnCurrentThread

The assertion seems to be some kind of poor man's thread safety check. It doesn't work with the previous patch because we might pass in a labeled event target even when we're running inside an unlabeled runnable. Anyway, I found three of these places and then gave up since there are probably quite a few more, and the change seemed dangerous.

This new patch avoids these problems since it adds a new method with clearer semantics.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8867379 [details] [diff] [review]
patch, v2

Review of attachment 8867379 [details] [diff] [review]:
-----------------------------------------------------------------

I'll be kind of curious to see how this gets used in practice.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> Comment on attachment 8867379 [details] [diff] [review]
> patch, v2
> 
> Review of attachment 8867379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll be kind of curious to see how this gets used in practice.

FYI, parts 2 and 3 in bug 1359833 depend on it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca9b0021c5b
Add a method to ask whether it's safe to run code related to a given SchedulerGroup (r=froydnj)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36b97ad6470
Revert "Bug 1363474 - Add a method to ask whether it's safe to run code related to a given SchedulerGroup (r=froydnj)"

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eec00055c9c
Add a method to ask whether it's safe to run code related to a given SchedulerGroup (r=froydnj)

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7eec00055c9c