Closed Bug 284389 Opened 20 years ago Closed 19 years ago

[FIX]revokeEvents doesn't always work correctly

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file)

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...
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.
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).
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?
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?
> 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.
> 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?
Attached patch Proposed fixSplinter Review
Attachment #180494 - Flags: superreview?(dbaron)
Attachment #180494 - Flags: review?(darin)
Attachment #180494 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 180494 [details] [diff] [review]
Proposed fix

makes sense, r=darin
Attachment #180494 - Flags: review?(darin) → review+
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.
Attachment #180494 - Flags: approval1.8b2?
Assignee: dougt → bzbarsky
Priority: -- → P1
Summary: revokeEvents doesn't always work correctly → [FIX]revokeEvents doesn't always work correctly
Target Milestone: --- → mozilla1.8beta2
Comment on attachment 180494 [details] [diff] [review]
Proposed fix

a=asa
Attachment #180494 - Flags: approval1.8b2? → approval1.8b2+
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: