284389 - [FIX]revokeEvents doesn't always work correctly

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See the comment at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/view/src/nsViewManager.cpp&rev=3.388&mark=361-362#360

I tried to code-inspect this away again, and I think I see a way that this could
happen.  When an event queue is being popped,
nsEventQueueServiceImpl::PopThreadEventQueue will:

1)  Call StopAcceptingEvents() on the queue
2)  Call ProcessPendingEvents() on the queue

Consider the case when processing one of these events causes a call to
revokeEvents on the thread on which we're popping the event queue.  The caller
will, typically, get the thread event queue, and call revokeEvents on it.  The
thing is, nsEventQueueServiceImpl::GetThreadEventQueue will get the youngest
queue that's still accepting events.  Since the queue being popped is no longer
accepting events, an ancestor of it will get returned, and hence events that
were posted to it will not get revoked.

Per blame, we used to return just the youngest queue.  The change to returning
the youngest active one was made by darin in revision 1.34 of
nsEventQueueService; the CVS comment is: "freeze nsIChannel nsIRequest".  The
change doesn't seem to be part of the diffs in the relevant bug.

Darin, do you know why you made this change?  Given that inactive queues forward
event posting to their ancestors, it seems that it should be save to always
return the youngest event queue, event if it's inactive, for GetThreadEventQueue...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Although perhaps the real issue is that the implementation of
nsEventQueueImpl::GetYoungestActive doesn't match the idl.  The idl says:

 75     /**
 76      * Fetch (and addref) the youngest member of the chain which is
 77      * still accepting events, or at least still contains events in need
 78      * of processing.

while the implementation does:

610                if (mYoungerQueue)
611 pavlov   3.26    mYoungerQueue->GetYoungestActive(getter_AddRefs(answer));
612                if (!answer) {
613                  if (mAcceptingEvents && mCouldHaveEvents)
614 danm     3.9       answer = NS_STATIC_CAST(nsIEventQueue *, this);
615 pavlov   3.26  }

Per the idl, that should be an ||, not an &&, I think.  Looking at blame, this
has always been an &&, since danm added mCouldHaveEvents in revision 3.8 of
nsEventQueue.cpp.

Comment 2

[Darin Fisher]

bz: I made that change (rev 1.34) based on my reading of the comment for that
function:

  // Return the active event queue on our chain

I also probably made it because I was hitting a bug of some sort.  (I remember
problems with PAC during startup, but I could be mistaken.)  At any rate, take a
look at my change for revision 1.35 of nsEventQueueService.cpp:

  +    aQueue->ProcessPendingEvents(); // make sure we don't orphan any events

In other words, previously PopThreadEventQueue did not call
ProcessPendingEvents.  See bug 135547 (but it's also pretty sparse on info).

Comment 3

[Darin Fisher]

bz: my last comment doesn't really answer your question... but what I vaguely
remember now is that we could sometimes get into a situation where the
application would hold onto a younger inactive event queue for the length of the
application.

what does it mean to call StopAcceptingEvents on a younger event queue that is
already inactive?  what if the caller really meant to de-activate the ancestor?

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the first question, per comment 2, is what it means for a queue to be
"active".  If a queue is not accepting events anymore but is still processing
events, is it active?

Second question is what the idl for nsIEventQueue should really be saying (that
is, whether the idl or the implementation is wrong).

Third question is how we make it possible to revoke events in a reasonable
way...  That's the part I really care about.  Perhaps we need a
"revokeEventsOnYoungestQueueWithEvents" method on nsIEventQueue or something?  ;)

For comment 3, the only caller of stopAcceptingEvents is the event queue
service.  Is there a reason not to move it to nsPIEventQueue or something?  Is 
there ever a reason for a caller to stop accepting events on random queues that
have younger queues that have not been popped yet?

Comment 5

[Darin Fisher]

> So the first question, per comment 2, is what it means for a queue to be
> "active".  If a queue is not accepting events anymore but is still processing
> events, is it active?

hmm... perhaps.  this is a very tricky complex system.


> Second question is what the idl for nsIEventQueue should really be saying (that
> is, whether the idl or the implementation is wrong).

we should make the IDL and impl consistent at any rate ;-)


> Third question is how we make it possible to revoke events in a reasonable
> way...  That's the part I really care about.  Perhaps we need a
> "revokeEventsOnYoungestQueueWithEvents" method on nsIEventQueue or something?  ;)

ugh.. i don't know.  maybe revokeEvents should be that function?


> For comment 3, the only caller of stopAcceptingEvents is the event queue
> service.  Is there a reason not to move it to nsPIEventQueue or something?  Is 
> there ever a reason for a caller to stop accepting events on random queues that
> have younger queues that have not been popped yet?

StopAcceptingEvents seems like a useful element of the API if someone is coding
to nsIEventQueue directly.  I'm not sure there is point in moving it to a
private API.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> we should make the IDL and impl consistent at any rate ;-)

I agree; I just don't know enough about the implications to know what changing
the impl would mean... Perhaps it's easiest to change the idl to conform.  :(

> ugh.. i don't know.  maybe revokeEvents should be that function?

That would make some sense.  I can't really think of a situation in which you'd
want to revoke events for a queue and its elder queues but not its younger
queues...  Can anyone else think of such a situation?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 8

[Darin Fisher]

Comment on attachment 180494 [details] [diff] [review]
Proposed fix

makes sense, r=darin

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180494 [details] [diff] [review]
Proposed fix

Requesting 1.8 approval. This should be a pretty low-risk patch that makes
revokeEvents actually revoke them reliably.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 180494 [details] [diff] [review]
Proposed fix

a=asa

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed.