From ae5b2d68ca402d72f243fb75c47d55ae6ebb49b7 Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Wed, 25 May 2022 23:36:41 +0200 Subject: [PATCH] AutoSuspend: Don't send LeaveStandby events from a zombie plugin instance (#9124) Long story short: the LeaveStandby event is sent via `tickAfterNext`, so if we tear down the plugin right after calling it (in this case, that means that the very input event that wakes the device up from suspend is one that kills ReaderUI or FileManager), what's in UIManager's task queue isn't the actual function, but the anonymous nextTick wrapper constructed by `tickAfterNext` (c.f., https://github.com/koreader/koreader/issues/9112#issuecomment-1133999385). Tweak `UIManager:tickAfterNext` to return a reference to said wrapper, so that we can store it and unschedule that one, too, in `AutoSuspend:onCloseWidget`. Fix #9112 (many thanks to [@boredhominid](https://github.com/boredhominid) for his help in finding a repro for this ;)). Re: #8638, as the extra debugging facilities (i.e., ebb81b98451e2a8f54c46f51e861c19fdfb40499) added during testing might help pinpoint the root issue for that one, too. Also includes a minor simplification to `UIManager:_checkTasks`, and various other task queue related codepaths (e.g., `WakeupMgr`) ;). --- .../apps/reader/modules/readerdictionary.lua | 2 +- frontend/device/wakeupmgr.lua | 4 +- frontend/ui/uimanager.lua | 49 +++++++++++-------- frontend/ui/widget/confirmbox.lua | 2 +- frontend/ui/widget/infomessage.lua | 2 +- plugins/autosuspend.koplugin/main.lua | 20 ++++++-- 6 files changed, 50 insertions(+), 29 deletions(-) diff --git a/frontend/apps/reader/modules/readerdictionary.lua b/frontend/apps/reader/modules/readerdictionary.lua index f78f2c508..802c4b2c5 100644 --- a/frontend/apps/reader/modules/readerdictionary.lua +++ b/frontend/apps/reader/modules/readerdictionary.lua @@ -996,7 +996,7 @@ function ReaderDictionary:showDict(word, results, boxes, link) if not results.lookup_cancelled and self._lookup_start_time and (time.now() - self._lookup_start_time) > self.quick_dismiss_before_delay then -- If the search took more than a few seconds to be done, discard - -- queued and coming up input events to avoid a voluntary dismissal + -- queued and upcoming input events to avoid a voluntary dismissal -- (because the user felt the result would not come) to kill the -- result that finally came and is about to be displayed Input:inhibitInputUntil(true) diff --git a/frontend/device/wakeupmgr.lua b/frontend/device/wakeupmgr.lua index 46dc43c6a..724864cdb 100644 --- a/frontend/device/wakeupmgr.lua +++ b/frontend/device/wakeupmgr.lua @@ -88,7 +88,7 @@ with anonymous functions. @treturn bool (true if one or more tasks were removed; false otherwise; nil if the task queue is empty). --]] function WakeupMgr:removeTasks(epoch, callback) - if #self._task_queue < 1 then return end + if #self._task_queue == 0 then return end local removed = false local reschedule = false @@ -147,7 +147,7 @@ If necessary, the next upcoming task (if any) is scheduled on exit. @treturn bool (true if we were truly woken up by the scheduled wakeup; false otherwise; nil if there weren't any tasks scheduled). --]] function WakeupMgr:wakeupAction(proximity) - if #self._task_queue > 0 then + if self._task_queue[1] then local task = self._task_queue[1] if self:validateWakeupAlarmByProximity(task.epoch, proximity) then task.callback() diff --git a/frontend/ui/uimanager.lua b/frontend/ui/uimanager.lua index 6ff885981..7f911cefb 100644 --- a/frontend/ui/uimanager.lua +++ b/frontend/ui/uimanager.lua @@ -521,13 +521,13 @@ function UIManager:close(widget, refreshtype, refreshregion, refreshdither) end end --- schedule an execution task, task queue is in ascendant order +-- Schedule an execution task; task queue is in ascending order function UIManager:schedule(sched_time, action, ...) local p, s, e = 1, 1, #self._task_queue if e ~= 0 then - -- do a binary insert + -- Do a binary insert. repeat - p = math.floor((e + s) / 2) -- Not necessary to use (s + (e -s) / 2) here! + p = bit.rshift(e + s, 1) -- Not necessary to use (s + (e -s) / 2) here! local p_time = self._task_queue[p].time if sched_time > p_time then if s == e then @@ -544,12 +544,13 @@ function UIManager:schedule(sched_time, action, ...) end e = p else - -- for fairness, it's better to make p+1 is strictly less than - -- p might want to revisit here in the future + -- For fairness, it's better to make sure p+1 is strictly less than p. + -- Might want to revisit that in the future. break end until e < s end + table.insert(self._task_queue, p, { time = sched_time, action = action, @@ -602,6 +603,9 @@ Useful to run UI callbacks ASAP without skipping repaints. @func action reference to the task to be scheduled (may be anonymous) @param ... optional arguments passed to action + +@return A reference to the initial nextTick wrapper function, +necessary if the caller wants to unschedule action *before* it actually gets inserted in the task queue by nextTick. @see nextTick ]] function UIManager:tickAfterNext(action, ...) @@ -609,7 +613,14 @@ function UIManager:tickAfterNext(action, ...) -- c.f., http://lua-users.org/wiki/VarargTheSecondClassCitizen local n = select('#', ...) local va = {...} - return self:nextTick(function() self:nextTick(action, unpack(va, 1, n)) end) + -- We need to keep a reference to this anonymous function, as it is *NOT* quite `action` yet, + -- and the caller might want to unschedule it early... + local action_wrapper = function() + self:nextTick(action, unpack(va, 1, n)) + end + self:nextTick(action_wrapper) + + return action_wrapper end --[[ -- NOTE: This appears to work *nearly* just as well, but does sometimes go too fast (might depend on kernel HZ & NO_HZ settings?) @@ -1142,7 +1153,7 @@ end --]] function UIManager:getNextTaskTime() - if #self._task_queue > 0 then + if self._task_queue[1] then return self._task_queue[1].time - time:now() else return nil @@ -1153,25 +1164,21 @@ function UIManager:_checkTasks() self._now = time.now() local wait_until = nil - -- task.action may schedule other events + -- Tasks due for execution might themselves schedule more tasks (that might also be immediately due for execution ;)). + -- Flipping this switch ensures we'll consume all such tasks *before* yielding to input polling. self._task_queue_dirty = false - while true do - if #self._task_queue == 0 then - -- Nothing to do! - break - end - local next_task = self._task_queue[1] - local task_time = next_task.time or 0 + while self._task_queue[1] do + local task_time = self._task_queue[1].time if task_time <= self._now then - -- remove from table + -- Pop the upcoming task, as it is due for execution... local task = table.remove(self._task_queue, 1) - -- task is pending to be executed right now. do it. - -- NOTE: be careful that task.action() might modify - -- _task_queue here. So need to avoid race condition + -- ...so do it now. + -- NOTE: Said task's action might modify _task_queue. + -- To avoid race conditions and catch new upcoming tasks during this call, + -- we repeatedly check the head of the queue (c.f., #1758). task.action(unpack(task.args, 1, task.argc)) else - -- queue is sorted in ascendant order, safe to assume all items - -- are future tasks for now + -- As the queue is sorted in ascending order, it's safe to assume all items are currently future tasks. wait_until = task_time break end diff --git a/frontend/ui/widget/confirmbox.lua b/frontend/ui/widget/confirmbox.lua index 181769420..fd892031a 100644 --- a/frontend/ui/widget/confirmbox.lua +++ b/frontend/ui/widget/confirmbox.lua @@ -186,7 +186,7 @@ function ConfirmBox:onShow() return "ui", self[1][1].dimen end) if self.flush_events_on_show then - -- Discard queued and coming up input events to avoid accidental dismissal + -- Discard queued and upcoming input events to avoid accidental dismissal Input:inhibitInputUntil(true) end end diff --git a/frontend/ui/widget/infomessage.lua b/frontend/ui/widget/infomessage.lua index 2edddf552..5c5888a78 100644 --- a/frontend/ui/widget/infomessage.lua +++ b/frontend/ui/widget/infomessage.lua @@ -234,7 +234,7 @@ function InfoMessage:onShow() return "ui", self[1][1].dimen end) if self.flush_events_on_show then - -- Discard queued and coming up input events to avoid accidental dismissal + -- Discard queued and upcoming input events to avoid accidental dismissal Input:inhibitInputUntil(true) end -- schedule us to close ourself if timeout provided diff --git a/plugins/autosuspend.koplugin/main.lua b/plugins/autosuspend.koplugin/main.lua index 3fd551f4f..db868019d 100644 --- a/plugins/autosuspend.koplugin/main.lua +++ b/plugins/autosuspend.koplugin/main.lua @@ -33,11 +33,12 @@ local AutoSuspend = WidgetContainer:new{ auto_suspend_timeout_seconds = default_auto_suspend_timeout_seconds, auto_standby_timeout_seconds = default_auto_standby_timeout_seconds, last_action_time = 0, - is_standby_scheduled = false, + is_standby_scheduled = nil, task = nil, standby_task = nil, leave_standby_task = nil, - going_to_suspend = false, + wrapped_leave_standby_task = nil, + going_to_suspend = nil, } function AutoSuspend:_enabledStandby() @@ -135,6 +136,10 @@ function AutoSuspend:init() if Device:isPocketBook() and not Device:canSuspend() then return end + -- We only want those to exist as *instance* members + self.is_standby_scheduled = false + self.going_to_suspend = false + UIManager.event_hook:registerWidget("InputEvent", self) -- We need an instance-specific function reference to schedule, because in some rare cases, -- we may instantiate a new plugin instance *before* tearing down the old one. @@ -199,7 +204,14 @@ function AutoSuspend:_unschedule_standby() end -- Make sure we don't trigger a ghost LeaveStandby event... + if self.wrapped_leave_standby_task then + logger.dbg("AutoSuspend: unschedule leave standby task wrapper") + UIManager:unschedule(self.wrapped_leave_standby_task) + self.wrapped_leave_standby_task = nil + end + if self.leave_standby_task then + logger.dbg("AutoSuspend: unschedule leave standby task") UIManager:unschedule(self.leave_standby_task) end end @@ -326,6 +338,8 @@ end function AutoSuspend:onLeaveStandby() logger.dbg("AutoSuspend: onLeaveStandby") + -- If the Event got through, tickAfterNext did its thing, clear the reference to the initial nextTick wrapper... + self.wrapped_leave_standby_task = nil -- Unschedule suspend and shutdown, as the realtime clock has ticked self:_unschedule() -- Reschedule suspend and shutdown (we'll recompute the delay based on the last user input, *not* the current time). @@ -587,7 +601,7 @@ function AutoSuspend:AllowStandbyHandler() -- (in case we were woken up by user input, as opposed to an rtc wake alarm)! -- (This ensures we'll use an up to date last_action_time, and that it only ever gets updated from *user* input). -- NOTE: UIManager consumes scheduled tasks before input events, which is why we can't use nextTick. - UIManager:tickAfterNext(self.leave_standby_task) + self.wrapped_leave_standby_task = UIManager:tickAfterNext(self.leave_standby_task) end end