54371 - nested event queues are broken

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

Currently, when you call ProcessPendingEvents on an nsIEventQueue, it
process's itself, then walks up the chain of assessors processing each.
These seams very wrong to me and I can do not understand how nested
event queues have ever worked.

What you would hope to have, is a call on ProcessPendingEvents which
either nook's if there are child event queues or calls through to the
youngest child.  Certainly, calling up is the wrong way to go.

Now, this in itself is not 'that' bad.  The major problem is how native
notifications happen.  When we nest an event, a listener registers the
new event with the gtk select list.  When we post an event, GTK is see
the event (e.g. a write on a pipe) and calls a handler.  The handler
calls ProcessPendingEvents on the registered EventQ.  This eventQ will
call up to this assessor and void any event protection!!!

I have corrected ProcessPendingEvents and fixed up the GTK
nsAppShell.cpp.  This should fix most/all problems that we have been
seeing with out of order events.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch to fix nested event queue

Comment 2

[Doug Turner (:dougt)]

Attachment: patch to fix nested event queue

Comment 3

[Doug Turner (:dougt)]

Attachment: Fixes Appshell dependancies on bad behavior

Comment 4

[Doug Turner (:dougt)]

Attachment: Cleans up gtk appshell.

Comment 5

[Doug Turner (:dougt)]

adding to cc-list.

Comment 6

[Dan M]

My take on the above patches: Now that I know they're predicated on earlier 
changes that removed a really important piece of ProcessPendingEvents, I'm leery 
of them.

But specifically, all of patch 15677 is just removing the pushed event queue 
during content window creation. That would be a problem if you restored that 
missing check in ProcessPendingEvents. And that missing check is important even 
outside the context of stacked queues; it really has to go back in. So patch 
15677 is broken.

Patch 15678 disables native notifications for stacked event queues on Linux. You 
mentioned that this is no longer necessary. But I missed something somewhere, and 
would like to understand how before I sign off on that patch.

The other two patches, 15617 and 15618, are the same, aren't they? I understand 
the need to remove processing events for elder queues (though since we've done 
that for ages without problems until recently, would this still be necessary if 
ProcessPendingEvents were fixed?). But I don't understand why you didn't simply 
remove the elder queue crawl, but also added new code which goes to the youngest 
queue, rather than processing events on the current queue. That way, it's 
possible that even after explicitly requesting event processing on a particular 
queue, it wouldn't. Again, I can imagine circumstances where this might be a good 
change, but I don't understand it. That change isn't covered by the stated desire 
to simply stop the elder queue crawl. Is it?

Comment 7

[Doug Turner (:dougt)]

Regarding the 15618 (which is the same as 15617), walking the elder list is
totally wrong that I believe we are in agreement about.  Now, what I have done
is to check to see in any event queues are pushed on top of the queue that is
being called.  I do this by getting the youngest queue.  This is a benefit
because embedders now only have to worry about registering ONE eventqueue with
their OS.

Based on 15618, 15678 just removes the now unneeded second registration of other
event queues with GTK.  

Now 15677, is the one I do not understand and went to you in the first place. 
Why would you want to block all events from the parent window while bringing up
a content window.  DanM has provided me with a test case which "should" break if
this pushed event queue is removed.  It does not.  

Comment 8

[Doug Turner (:dougt)]

Attachment: Replaces check for processingEvents in plevent.c

Comment 9

[Dan M]

  Ah. OK. I see where you're wanting to go with the "let me worry about just one 
event queue" thing with 15618. It didn't work with our app. I guess it could work 
for an embedding app, but only if they were careful what they did with their one 
event queue. In general, it's a dangerous thing to try, and everyone but Necko 
should always just ask the event queue service for the current event, rather than 
trying to cache their own.
  I don't understand your reasoning behind removing the code in 15678, even if we 
went with 15618. It turns off the notification of posted events for all but the 
original queue. Sure, once we're in ProcessPendingEvents, with 15618, we'll 
process the youngest queue, but it's the code removed by 15678 that gets us into 
ProcessPendingEvents in the first place, when events are posted on any but the 
eldest queue. I don't see how 15618 would help us pick up events that 15678 would 
drop.
  As for my test case that didn't show any problems with patch 15677, well, that 
was because of the bustage in ProcessPendingEvents since rev 1.11. Unbreak that, 
and 15677 should cause a lockup in esoteric cases, like windows with window.open 
in their onload handler, for instance. Or content windows opened from netlib, if 
such a thing ever happens.

Comment 10

[Doug Turner (:dougt)]

16122 should get checked in.  I think that we all agree on this point.  It
should be checked into both the trunk and the 


As for that rest of the fixes - I do break nsXULWindow.cpp.  I am not sure what
to do here (other than backing off checking this in).

The general idea was to make it possible to register only ONE event queue with
the native OS.  This would allow embedders not to worry about modal dialogs too
much.   If there was only one event queue to worry about, the gtk event handler
would have to make sure it calls the lowest event queue (eg the last one
pushed).  The only way to do this was to change the way
nsEventQueue::ProcessPendingEvents worked.  It had to call through to the
youngest child.  I could have moved this logic out and made the event queue
chain a public api.

Comment 11

[Doug Turner (:dougt)]

PDT TEAM - We need to check in patch 16122 into the branch.  It fixes numberous
event-out-of-order bugs.  It is a regression I caused early last month.  hyatt
danm and I reviewed it.  I am going to check it into the trunk today.

It is low risk since the check which I erronously removed exists in this
function for years.

Comment 12

[Dan M]

Because my current understanding of the rulez is that PDT won't accept one 
engineer's vouching for another's review, I'm making it explicit: 16122 has to go 
in. r=me. Now I suppose we need one of the blessed reviewers to rubberstamp us.

Comment 13

[Darin Fisher]

Patch 16122 seems to fix all of the necko bugs related to nested
event handlers.  I would recommend this patch for rtm.

I've verified that it fixes: bug 53429, bug 52061, bug 52949, bug 42624,
bug 52734, and bug 54597 against the branch CVS as of 10/4/00.

Comment 14

[Brendan Eich [:brendan]]

Ok, I have been hanging back, working on other stuff, probably ruining my rep 
with dougt (sorry, FWIW), and waiting for this to get settled.  With my own head 
hung low, I can a=[e-mail] that patch 
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16122).  Let's get it 
into the trunk ASAP.

/be

Comment 15

[Doug Turner (:dougt)]

fixed on trunk:

Checking in plevent.c;
/cvsroot/mozilla/xpcom/threads/plevent.c,v  <--  plevent.c
new revision: 1.14; previous revision: 1.13
done

Comment 16

[Phil Peterson]

I was gonna mark rtm++ to put this on the branch, but it looks like Darin took
off the rtm+ on this bug. Is there a reason for that? Sure seems like something
we'd want for RTM...

Comment 17

[Phil Peterson]

rtm++

Comment 18

[Doug Turner (:dougt)]

A fix check in on both branch and trunk.  

Comment 19

[Darin Fisher]

I did not mean to take off the rtm+ on this bug.  I think the patch is something
that really should go into RTM.

Comment 20

[Doug Turner (:dougt)]

So the patch to plevent.c is in.  I do however still think that nested event
queues and for that matter event processing needs to be cleaned up.  The first
three patches were inching toward where I think we need to go.  Dan and I will
meet later and talk about what we can do to clean up our story for embedders.

Comment 21

[selmer (gone)]

Is this checked in on the _branch_?  The rtm++ is gone from the whiteboard so
this is still in the list of rtm bugs.  If the rtm portion of this is done,
perhaps this should be split into 2 bugs - one for the rtm fix and another for
the rest.  Anyway, please update this bug to the appropriate state.

Comment 22

[selmer (gone)]

rtm-, no response, no complaints.

Comment 23

[Dan M]

2 

Comment 24

[Darin Fisher]

*** Bug 52397 has been marked as a duplicate of this bug. ***

Comment 25

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Darin: I know your nsIThreadManager changes affect this bug, but I'm not sure whether they make it INVALID, WORKSFORME or FIXED.

Comment 27

[Darin Fisher]

Marking as invalid due to bug 326273.  This bug refers to APIs that no longer exist.