ThrottledEventQueue::Inner::IsOnCurrentThread has unexpected semantics

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Comment 1

2 years ago
It is very unexpected that it uses anything else than thread information for the result of a method called IsOnCurrentThread.
(Assignee)

Comment 2

2 years ago
Ben, what is the reason for the really weird behavior? Does some code rely on it?
Flags: needinfo?(bkelly)
Summary: ThrottledEventQueue::IsOnCurrentThread has unexpected semantics → IsOnCurrentThread in ThrottledEventQueue has unexpected semantics
(Assignee)

Comment 3

2 years ago
Er, it is ThrottledEventQueue::Inner::IsOnCurrentThread
Summary: IsOnCurrentThread in ThrottledEventQueue has unexpected semantics → ThrottledEventQueue::Inner::IsOnCurrentThread has unexpected semantics
(Assignee)

Comment 4

2 years ago
But the behavior is exposed also to ThrottledEventQueue::IsOnCurrentThread
(Assignee)

Updated

2 years ago
Blocks: 1300659
(Assignee)

Comment 5

2 years ago
I assume the weird and against the API contract mExecutionDepth was added because of some reason, but couldn't figure out why, so decided to push this to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f185773353bdaf51f3d82b27e183a2d2777a79f9
Why is that broken?  I would expect nsIEventTarget::IsOnCurrentThread() to report if the target is executing on the current thread.  That is what ThrottledEventQueue implements.

Are you expecting some other semantic from nsIEventTarget::IsOnCurrentThread()?
Flags: needinfo?(bkelly)
Once bug 1361164 lands, the semantics will be clearer.

IsOnCurrentThread: returns true only if an event that was dispatched through the event target is currently running.
DispatchesToCurrentThread: returns true if the event target dispatches events to the current thread.
I guess the idl does suggest ThrottledEventQueue is wrong:

https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsIEventTarget.idl#56

I find the name very confusing, though.

Anyway, I don't think anything depends on this specifically.  It was just my misunderstanding of the API.
Comment on attachment 8872184 [details] [diff] [review]
throttled_queue.diff

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

LGTM based on further reading of the nsIEventTarget.idl contract.  I don't think anything depended on this behavior.
Attachment #8872184 - Flags: review+
Oh.  I just saw comment 7.

Olli, it seems you want the DispatchedToCurrentThread() behavior?

Should we just wait for that instead of ripping this code out of ThrottledEventQueue only to put it back?
Flags: needinfo?(bugs)
Comment on attachment 8872184 [details] [diff] [review]
throttled_queue.diff

Dropping my r+ because I'm not sure on the desired path based on comment 7 and bug 1361164.
Attachment #8872184 - Flags: review+
(Assignee)

Comment 12

2 years ago
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #8)
> I guess the idl does suggest ThrottledEventQueue is wrong:
Exactly. And we had changed the semantics of an old API basically in one place.
Flags: needinfo?(bugs)
(Assignee)

Comment 13

2 years ago
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #10)
> Should we just wait for that instead of ripping this code out of
> ThrottledEventQueue only to put it back?


No, all this idle stuff is super high priority to get working well.
We have tons of stuff waiting for it.

And sounds like the semantics are really weird after bug 1361164. We really should change the
method name if we totally change the semantics.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 14

2 years ago
bug 1361164 adds a comment about ThrottledEventQueue being special, and it definitely shouldn't be special comparing to other event targets.
(Assignee)

Updated

2 years ago
Attachment #8872184 - Flags: review?(bkelly)
(Assignee)

Updated

2 years ago
Assignee: nobody → bugs
(Assignee)

Comment 15

2 years ago
I think I'm landing the patch. It makes ThrottledEventQueue work more consistently with the other event targets.
And I don't see anything in the patch for bug 1361164 to change the semantics to what comment 7 talks about.
Attachment #8872184 - Flags: review?(bkelly) → review+

Comment 16

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4381d9011522
ThrottledEventQueue::Inner::IsOnCurrentThread should work similarly to other IsOnCurrentThread implementations, r=bkelly

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4381d9011522
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.