1368358 - ThrottledEventQueue::Inner::IsOnCurrentThread has unexpected semantics

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

https://bugzilla.mozilla.org/show_bug.cgi?id=1311425#c63

Comment 1

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

It is very unexpected that it uses anything else than thread information for the result of a method called IsOnCurrentThread.

Comment 2

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

Ben, what is the reason for the really weird behavior? Does some code rely on it?

Comment 3

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

Er, it is ThrottledEventQueue::Inner::IsOnCurrentThread

Comment 4

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

But the behavior is exposed also to ThrottledEventQueue::IsOnCurrentThread

Comment 5

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

Attachment: throttled_queue.diff

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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()?

Comment 7

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

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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 9

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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 11

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 12

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

(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.

Comment 13

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

(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.

Comment 14

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

bug 1361164 adds a comment about ThrottledEventQueue being special, and it definitely shouldn't be special comparing to other event targets.

Comment 15

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

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4381d9011522
ThrottledEventQueue::Inner::IsOnCurrentThread should work similarly to other IsOnCurrentThread implementations, r=bkelly

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4381d9011522