76075 - nsEventQueueImpl::EventAvailable() should check mElderQueue

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Simon Fraser [no longer active]]

One would think that you could test the result of 
nsEventQueueImpl::EventAvailable() to determine if you need to spend time calling 
nsEventQueueImpl::ProcessPendingEvents(). However, this seems to break things, 
afer a modal dialog has been shown (perhaps because we are leaking event queues).

It seems that nsEventQueueImpl::EventAvailable() should also test for available 
events on mElderQueue.

Comment 1

[Simon Fraser [no longer active]]

Attachment: nsEventQueue patch

Comment 2

[Simon Fraser [no longer active]]

I think that patch does the right thing.

Comment 3

[Dan M]

  The whole point of the queue stacks is so events can queue up in elder queues 
and not be processed. So barring inactive queues leaking, it doesn't really make 
sense to ask up the whole chain. However, this wouldn't be our only hack around 
leaking event queues. Add a comment to that effect, please, and r=me.
  The patch as it stands will tend to overreport waiting events in the legitimate 
case when the youngest queue is active and elder queues are queueing up events 
that they won't let go of even if asked. But I suppose that won't hurt anything.

Comment 4

[Doug Turner (:dougt)]

I don't think I like this.  Suppose that we are running with Simon's patch.  
Also suppose that there is a nested event queue.  The elder queue has pending 
events, and the child queue is empty and will not receive any more events.  Ask 
if there is an event available on the child queue.  It will return an event on 
the elder queue by our supposition.  Now, call GetEvent.  This function will 
return NULL which breaks the symantecs of this api.

Am I missing something?

Comment 5

[Simon Fraser [no longer active]]

The symantecs of the API are already broken. These two should have the same 
outcome:

   if (queue->EventAvailable())
     queue->ProcessPendingEvents()

and

   queue->ProcessPendingEvents().

They currently do not.

It would seem that the crock here is hiding nested event queues behind 
nsIEventQueue.

Comment 6

[Dan M]

  Doug's comment highlights the difference between GetEvent and 
ProcessPendingEvents. Currently, EventAvailable reflects the state of GetEvent, 
but not ProcessPendingEvents. With Simon's patch, EventAvailable would reflect 
the state of ProcessPending, but lie about GetEvent. Yup. I guess it depends on 
which state you care about most. I'm not familiar enough with the uses of 
EventAvailable to know which use you really want. Maybe you should add an 
EventPending method. Confuses even me.
  NB: you don't want to make GetEvent crawl elder queues. It ignores the 
ProcessPending lockout. And I'm not familiar enough with where we're using that 
either to judge offhand whether it should start paying attention. Could look, I 
guess. But it sounds dangerous.

Comment 7

[Doug Turner (:dougt)]

so currently, EventAvailable, GetEvent, HandleEvent share the same symantecs of
not paying attention to the elder queue.  

What is the benefit of changing this?  Is there a bustage that I can try to
reproduce?

Comment 8

[Simon Fraser [no longer active]]

I need this to be fixed so that I can use EventAvailable() to tell me whether 
there are any PLEvents waiting to be processed, which would indicate that the 
browser is 'busy', and we should not yield too much time to the OS (see the 
dependent bug).

Comment 9

[Doug Turner (:dougt)]

The only usage that you *could* break by this is one of the oji tests:

modules/oji/tests/src/TM/PostEvent.cpp

If that doesn't break, r=dougt.

I am going to reassign back to you simon, so that you can have the glory (and 
blame).  :-)

Comment 10

[Dan M]

Don't forget the comment that it's a hack that shouldn't be necessary if not for 
leaking queues.

Comment 11

[Simon Fraser [no longer active]]

I don't think leaking event queues were the issue here. I think what happened is 
that, during my broken attempt to use 

   if (queue->EventAvailable())
     queue->ProcessPendingEvents()

I got a hidden zombie dialog, which thus left chained event queues lying around. 
I need this fix whether event queues leak or not.

Comment 12

[Simon Fraser [no longer active]]

Hey, not fair for me to get this bug. How about some responsibility of ownership 
here?

The API as it stands has symantec issues. I need a method that will tell me if 
ProcessPendingEvents will do *any* event processing. Please give me one. Thanks.

Comment 13

[Doug Turner (:dougt)]

okay.  no problem.  how about this? we add a method:

boolean pendingEvents();

This will tell you if there are any events that could be processed by a call to 
|processPendingEvents|.  In this way, the symantecs are balanced.

Comment 14

[Simon Fraser [no longer active]]

I'd prefer either

readonly attribute boolean hasPendingEvents;

or boolean hasPendingEvents();

but that would be swell.

Comment 15

[Doug Turner (:dougt)]

Attachment: Proposed Patch v1

Comment 16

[Simon Fraser [no longer active]]

Patch works for me, sr=sfraser. Thanks doug

Comment 17

[Dan M]

I'll r= it if I get to bitch about you guys both misspelling "semantics" the same 
way. I wasn't going to say anything, but then you both did it. And Simon: 
referring to my comment on 4-16 (and yours on 4-18), this whole method should be 
unnecessary. I reiterate that it means something fundamental is broken. I still 
suspect leaking event queues.

Comment 18

[timeless]

And here i thought they were talking about the corporation

Comment 19

[Doug Turner (:dougt)]

Symantec and netwerk.  two misspelling that I will have to live with for a long 
time... :-)


Anyways.  Is this additional api proposol orthogonal to the your suspection of  
leaking event queues?  I don't understand what you mean by leaking event 
queues....

Comment 20

[Dan M]

  Referring back to a previous "stacked event queues are broken" discussion, we 
shouldn't need to process events on elder queues. All but the youngest should be 
accepting but not dispensing events. We check elder queues at all, only because 
of undead younger queues (leaks) and maybe edge cases during destruction. Even 
then, there's a secondary lockout on elder queues that prevents them from 
processing events. Without that we'd have worse problems.
  So given all that, it wouldn't make sense to check younger queues for available 
events unless there was a problem. However, as I said in my first comment in this 
bug, we do have problems, so it does make sense. I guess. I'm just lobbying for a 
comment in the source.

Comment 21

[Doug Turner (:dougt)]

can you add your comment to the patch and reattach?  

Comment 22

[Simon Fraser [no longer active]]

Ignore the leaking event queues problem. It is not an issue here. I mentioned it 
because I saw strange behaviour when changing the central place where Mac 
processes PLEvents from

   queue->ProcessPendingEvents().

to

   if (queue->EventAvailable())
     queue->ProcessPendingEvents()

which caused us to totally fail to process events in mElderQueue.

Comment 23

[Doug Turner (:dougt)]

I checked in the new method.  

Comment 24

[Dan M]

Let me try this one more time. The presence of this method implies an error 
somewhere. The error is probably a leaking event queue. A comment to that effect 
seems like a minor concession. Oh well.

Comment 25

[Simon Fraser [no longer active]]

Reopening this. It's too easy to bust event handling in strange and mysterious 
ways here. See bug 79158, the root cause of which was to change:

  eventQueue->ProcessPendingEvents();

to 

  if (eventQueue->PendingEvents())
    eventQueue->ProcessPendingEvents();

Why does this break? Is danm really right, that we are leaking event queues?

Comment 26

[Brendan Eich [:brendan]]

sfraser: heed danm's wise words of warning next time!

I have a question: why does event queue lifetime affect where events get queued
or processed? Shouldn't leaked queues be off in the weeds, not processed?  That
is, lifetime is governed by ref-count, but a queue has to be on its thread's
stack of event queues to have events enqueued in it.  And the code that pushes
and pops that stack pops when it should.

Do we have state to distinquish such "on stack" or "valid" or "open" queues from
"leaked" or "invalid" or "closed" ones?  If so, can we use it to destroy events
as callers attempt to enqueue them on closed queues?

Ok, what am I missing?

/be

Comment 27

[Dan M]

If it was wisdom I was spewing, I still didn't foresee a bland-looking patch 
causing problems like this. (I also don't understand the benefit -- with no 
events queued, ProcessPending should be nearly as fast as the Pending check.) 
Regardless, I can see only one difference skipping ProcessPending, and I haven't 
managed to put together a scenario where it would -- presumably -- cause events 
to languish.

By the way, we do distinguish between fully active queues and ones that are 
winding down and trying to go away. We try hard to accommodate misguided code 
that tries to post events to the darkened queue. Generally, it remains 
effectively active until we can truly get rid of it.

Comment 28

[Brendan Eich [:brendan]]

Would it help if we didn't hand out strong refs to individual queues, instead
requiring everyone to call an event-queue-stack or service method to put some
event on the top queue?  Or do we need to expose other places in the stack (top
and bottom, say; the classic codebase did this for main-to-mocha-thread event
queueing)?

I think so long as someone can leak a queue by holding a strong ref too long,
and have bad effects on event routine, we are doomed.

/be

Comment 29

[Dan M]

No, because this whole house of cards works predicated on a careful (or 
serendipitous, if you're feeling uncharitable) balance of users who sometimes get 
the current latest queue and sometimes cache one. In particular, a networking 
connection must continue to post events to the same queue it started with while 
it's stalled and other connections are going through on top of it.

Comment 30

[Christopher Blizzard (:blizzard)]

Or, even better, we could use the same queue all over the place and instead of
pushing and popping actual queues we could just know to remove some events from
the queue when going modal and push them back when we are done.  No more
ownership problems and it's a cleaner model IMHO.

Comment 31

[Brendan Eich [:brendan]]

Aesthetics about what's cleaner aside, the stack of event queues could be
cheaper, because you don't need to do O(queue-population) operations finding
events to remove for later re-insertion (which also costs similarly).  I created
a stack of event queues in 4.x, and it worked well enough, but we didn't hand
out strong refs to queues for people to abuse later.  Events either went into
the top queue, or the bottom.

danm, dougt: why do we need to give a strong ref to a "middle" queue to Necko,
what kind of events is it stuffing in that queue while a modal dialog flies over
top of that middle queue?

/be

Comment 32

[Doug Turner (:dougt)]

I can't think of any good reason.  The more I think about it, the more I think 
it is wrong to do this.  

Comment 33

[Doug Turner (:dougt)]

simon, can with push this bug out?  

Comment 34

[Simon Fraser [no longer active]]

Yeah, I removed the PendingEvents() check, and stuff works now. I'd still like us 
to get to the bottom of this tho.

Comment 35

[Doug Turner (:dougt)]

someone yell if they really think this is a show stopper.

Comment 36

[basic]

Changing platform

Comment 37

[timeless]

there is no live patch
<Doug T@2001-04-25 22:30> I checked in the new method.

Simon Fraser 2001-04-24 11:18 makes some strange comment about Mac, the 
original need was clearly for macos (see dep bug).

I'm marking this arch because that's about the only conclusion i can draw.

Comment 38

[Darin Fisher]

nsEventQueueImpl::EventAvailable() no longer exists.  see bug 326273.