Closed
Bug 54371
Opened 24 years ago
Closed 18 years ago
nested event queues are broken
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: dougt, Unassigned)
References
Details
(Keywords: arch, Whiteboard: [rtm-])
Attachments
(5 files)
948 bytes,
patch
|
Details | Diff | Splinter Review | |
948 bytes,
patch
|
Details | Diff | Splinter Review | |
2.69 KB,
patch
|
Details | Diff | Splinter Review | |
5.26 KB,
patch
|
Details | Diff | Splinter Review | |
540 bytes,
patch
|
Details | Diff | Splinter Review |
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•24 years ago
|
Reporter | ||
Comment 1•24 years ago
|
||
Reporter | ||
Comment 2•24 years ago
|
||
Reporter | ||
Comment 3•24 years ago
|
||
Reporter | ||
Comment 4•24 years ago
|
||
Reporter | ||
Comment 5•24 years ago
|
||
adding to cc-list.
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•24 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•24 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•24 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•24 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•24 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•24 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
Comment 14•24 years ago
|
||
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•24 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
Comment 16•24 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
Reporter | ||
Comment 18•24 years ago
|
||
A fix check in on both branch and trunk.
Comment 19•24 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•24 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
Comment 21•24 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 24•24 years ago
|
||
*** Bug 52397 has been marked as a duplicate of this bug. ***
Comment 25•23 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
Comment 26•18 years ago
|
||
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•18 years ago
|
||
Marking as invalid due to bug 326273. This bug refers to APIs that no longer exist.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•