Closed Bug 1368358 Opened 3 years ago Closed 3 years ago
Event Queue::Inner::Is On Current Thread has unexpected semantics
It is very unexpected that it uses anything else than thread information for the result of a method called IsOnCurrentThread.
Ben, what is the reason for the really weird behavior? Does some code rely on it?
Summary: ThrottledEventQueue::IsOnCurrentThread has unexpected semantics → IsOnCurrentThread in ThrottledEventQueue has unexpected semantics
Er, it is ThrottledEventQueue::Inner::IsOnCurrentThread
Summary: IsOnCurrentThread in ThrottledEventQueue has unexpected semantics → ThrottledEventQueue::Inner::IsOnCurrentThread has unexpected semantics
But the behavior is exposed also to ThrottledEventQueue::IsOnCurrentThread
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()?
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.
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?
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.
(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.
(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.
bug 1361164 adds a comment about ThrottledEventQueue being special, and it definitely shouldn't be special comparing to other event targets.
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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/4381d9011522 ThrottledEventQueue::Inner::IsOnCurrentThread should work similarly to other IsOnCurrentThread implementations, r=bkelly
You need to log in before you can comment on or make changes to this bug.