Closed
Bug 1398417
Opened 7 years ago
Closed 7 years ago
Fix PrioritizedEventQueue bugs with prioritized input events
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
Details
Attachments
(1 file, 1 obsolete file)
1.95 KB,
patch
|
stone
:
review+
|
Details | Diff | Splinter Review |
This patch fixes two issues. The first one is that we can enter PrioritizedEventQueue<InnerQueueT>::SelectQueue when mInputHandlingStartTime is null and aUpdateState is false. I think the easiest way to deal with that is to avoid using the input queue unless the other queues are empty. That way we don't have to worry about whether it's safe to call into the refresh driver. The second problem is a really dumb typo I made. The fix is obvious.
Attachment #8906219 -
Flags: review?(sshih)
Comment 1•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #0) > Created attachment 8906219 [details] [diff] [review] > patch > > This patch fixes two issues. The first one is that we can enter > PrioritizedEventQueue<InnerQueueT>::SelectQueue when mInputHandlingStartTime > is null and aUpdateState is false. I think the easiest way to deal with that > is to avoid using the input queue unless the other queues are empty. That > way we don't have to worry about whether it's safe to call into the refresh > driver. > > The second problem is a really dumb typo I made. The fix is obvious. Should we worry about the case that only some pending input events in the queue? In that case PrioritizedEventQueue<InnerQueueT>::HasReadyEvent will return false.
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Ming-Chou Shih [:stone] from comment #1) > (In reply to Bill McCloskey (:billm) from comment #0) > > Created attachment 8906219 [details] [diff] [review] > > patch > > > > This patch fixes two issues. The first one is that we can enter > > PrioritizedEventQueue<InnerQueueT>::SelectQueue when mInputHandlingStartTime > > is null and aUpdateState is false. I think the easiest way to deal with that > > is to avoid using the input queue unless the other queues are empty. That > > way we don't have to worry about whether it's safe to call into the refresh > > driver. > > > > The second problem is a really dumb typo I made. The fix is obvious. > > Should we worry about the case that only some pending input events in the > queue? Sorry, I don't understand this part. > In that case PrioritizedEventQueue<InnerQueueT>::HasReadyEvent will > return false. HasReadyEvent is only supposed to look at the first item in the queue. It's possible that the first item is not ready but other items behind it are ready. In that case, we want HasReadyEvent to return false. We're not allowed to process items in a single queue out of order, so we can't do anything if the first event is not ready. Maybe this isn't what you meant though?
Comment 3•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #2) > (In reply to Ming-Chou Shih [:stone] from comment #1) > > (In reply to Bill McCloskey (:billm) from comment #0) > > > Created attachment 8906219 [details] [diff] [review] > > > patch > > > > > > This patch fixes two issues. The first one is that we can enter > > > PrioritizedEventQueue<InnerQueueT>::SelectQueue when mInputHandlingStartTime > > > is null and aUpdateState is false. I think the easiest way to deal with that > > > is to avoid using the input queue unless the other queues are empty. That > > > way we don't have to worry about whether it's safe to call into the refresh > > > driver. > > > > > > The second problem is a really dumb typo I made. The fix is obvious. > > > > Should we worry about the case that only some pending input events in the > > queue? > > Sorry, I don't understand this part. > > > In that case PrioritizedEventQueue<InnerQueueT>::HasReadyEvent will > > return false. > > HasReadyEvent is only supposed to look at the first item in the queue. It's > possible that the first item is not ready but other items behind it are > ready. In that case, we want HasReadyEvent to return false. We're not > allowed to process items in a single queue out of order, so we can't do > anything if the first event is not ready. > > Maybe this isn't what you meant though? I'm thinking about the case that there are only pending events in the input event queue. In that case, we won't select the input event queue, return false for HasReadyEvent, and yield the current thread to others. The pending input events are processed when we put another event in other queues (high, normal, or idle). Maybe it's very short, so we don't need to worry about it?
Assignee | ||
Comment 4•7 years ago
|
||
Well, we do check the input queue a second time: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/xpcom/threads/PrioritizedEventQueue.cpp#168 So I don't think we need to worry about what happens when all other queues are empty. We'll still select the input queue. However, it seems like a bad idea to have HasReadyEvent work differently from GetEvent. In bug 1350432 comment 30, you said that this patch caused an NS_IsMainThread assertion in the refresh driver code. I'm not seeing it myself. In any case, I think we should fix this first and then fix that later if we need to. The scheduler isn't enabled right now.
Attachment #8906219 -
Attachment is obsolete: true
Attachment #8906219 -
Flags: review?(sshih)
Attachment #8906438 -
Flags: review?(sshih)
Comment 5•7 years ago
|
||
Comment on attachment 8906438 [details] [diff] [review] patch v2 LGTM, thanks.
Attachment #8906438 -
Flags: review?(sshih) → review+
Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8757f33153 Fix PrioritizedEventQueue bugs with input event prioritization (r=stone)
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec8757f33153
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•