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

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: aosmond, Assigned: billm)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1359833
Flags: needinfo?(wmccloskey)
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?
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey) → needinfo?(aosmond)
Attachment #8866565 - Flags: review?(nfroyd)
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.
Attachment #8866565 - Flags: review?(nfroyd)
(Reporter)

Comment 3

a year ago
(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.
Flags: needinfo?(aosmond)
Created attachment 8867379 [details] [diff] [review]
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.
Attachment #8866565 - Attachment is obsolete: true
Attachment #8867379 - Flags: review?(nfroyd)
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.
Attachment #8867379 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 6

a year ago
(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

a year ago
Pushed by wmccloskey@mozilla.com:
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

a year ago
Pushed by wmccloskey@mozilla.com:
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

a year ago
Pushed by wmccloskey@mozilla.com:
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)
https://hg.mozilla.org/mozilla-central/rev/7eec00055c9c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.