nested event queues are broken

RESOLVED INVALID

Status

()

Core
XPCOM
RESOLVED INVALID
18 years ago
12 years ago

People

(Reporter: dougt, Unassigned)

Tracking

({arch})

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-])

Attachments

(5 attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Blocks: 48064, 52667, 53429
Keywords: arch, nsbeta3
Priority: P3 → P1
(Reporter)

Comment 1

18 years ago
Created attachment 15617 [details] [diff] [review]
patch to fix nested event queue
(Reporter)

Comment 2

18 years ago
Created attachment 15618 [details] [diff] [review]
patch to fix nested event queue
(Reporter)

Comment 3

18 years ago
Created attachment 15677 [details] [diff] [review]
Fixes Appshell dependancies on bad behavior
(Reporter)

Comment 4

18 years ago
Created attachment 15678 [details] [diff] [review]
Cleans up gtk appshell.
(Reporter)

Updated

17 years ago
Blocks: 54648
(Reporter)

Comment 5

17 years ago
adding to cc-list.

Comment 6

17 years ago
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?
(Reporter)

Comment 7

17 years ago
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.  

(Reporter)

Comment 8

17 years ago
Created attachment 16122 [details] [diff] [review]
Replaces check for processingEvents in plevent.c

Comment 9

17 years ago
  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.
(Reporter)

Comment 10

17 years ago
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.

Keywords: rtm
(Reporter)

Comment 11

17 years ago
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.
Whiteboard: NEED RTM+ for branch

Comment 12

17 years ago
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

17 years ago
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.
Whiteboard: NEED RTM+ for branch
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=brendan@mozilla.org that patch 
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16122).  Let's get it 
into the trunk ASAP.

/be
(Reporter)

Comment 15

17 years ago
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

Updated

17 years ago
Blocks: 52397

Comment 16

17 years ago
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...
No longer blocks: 52397

Comment 17

17 years ago
rtm++
Whiteboard: [rtm++]
(Reporter)

Comment 18

17 years ago
A fix check in on both branch and trunk.  

Comment 19

17 years ago
I did not mean to take off the rtm+ on this bug.  I think the patch is something
that really should go into RTM.
Whiteboard: [rtm++]
(Reporter)

Comment 20

17 years ago
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.
Status: NEW → ASSIGNED

Updated

17 years ago
Blocks: 52397

Comment 21

17 years ago
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

17 years ago
rtm-, no response, no complaints.
Whiteboard: [rtm-]

Comment 23

17 years ago
2 
Assignee: dougt → danm
Status: ASSIGNED → NEW

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9

Comment 24

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

Updated

17 years ago
Target Milestone: mozilla0.9 → mozilla1.0

Comment 25

16 years ago
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)
Target Milestone: mozilla1.0 → mozilla1.0.1

Updated

16 years ago
Target Milestone: mozilla1.0.1 → ---

Updated

16 years ago
Target Milestone: --- → Future

Updated

13 years ago
Assignee: danm.moz → nobody
Status: ASSIGNED → NEW
Darin: I know your nsIThreadManager changes affect this bug, but I'm not sure whether they make it INVALID, WORKSFORME or FIXED.
Priority: P1 → --
QA Contact: rayw → xpcom
Target Milestone: Future → ---

Comment 27

12 years ago
Marking as invalid due to bug 326273.  This bug refers to APIs that no longer exist.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.