light refactor to the GAM to solve a deadlock issue

This is a "no good choice" situation but there is a problem with
progress bars.

You want progress bars to update quickly, and you don't want
to have to context switch out for the entire graphics stack to
come back to you to approve the drawing of your progress bar update.

On the other hand, you don't want to short-circuit all the checks
on drawing stuff to the screen for the sake of progress bars --
right now, there are a number of checks in place to make sure you
can't draw stuff on parts of the screen you shouldn't, to avoid
fake messages and dialog boxes.

I had managed to work out a way to get a dialog box to redraw without
violating the security rules -- each redraw is checked by the defacing
routine -- however, due to the way I was computing the height of
the modal I was causing the dialog box to be resized to its largest
size and then shrunk again to fit the text every iteration. This
in essence also caused the defacement routine to run repeatedly,
which caused the GAM to also issue repeated redraw requests to
the main loop.

These redraw requests can't be serviced until the task being tracked
by the progress bar is completed. As the progress bar gets redrawn
almost a hundred times as part of the task, roughly a hundred messages
arrive in the server's queue before it can handle a single one.

This results in a deadlock situation on completion of the progress
bar, because we want to fire off a message to ourselves to go to
the next state in the Ux routine.

There are a few possible solutions to this:

1. Multi thread the heck out of the routine doing the progress bar,
so the dialog box and the slider are each in their own threads
with their own responders and then the thing doing the computation
is doing some Mutex-guarded-Arc thing.

2. Do some very unsafe bodge to allow sliders to do direct draws
that are unchecked

3. Add a check so that if the size of the dialog box doesn't change,
defacement isn't recomputed and thus redraw messages aren't sent back.

(1) doesn't work for a number of reasons -- in a general case, it's
probably OK, but it does cause a lot more memory usage (and we are
lean on memory); but more importantly, for this use case, our routine
is interacting with CSRs and those don't implement Send so you can't
pass them into a thread via a closure. You could maybe put the dialog
boxes themselves into an Arc Mutex thing but because this routine
was heavily coded to re-use the heavyweight Ux resources this becomes
a bit cumersome to code.

(2) is terrible

(3) ended up working and what is in this commit. I think it's OK
because a progress bar generally doesn't require changing the
size of a window to update its state; therefore the redraw is
deface-safe (since you aren't changing the bounds of what you were
already approved to draw in). Of course, if you have some sort
of a progress bar implementation that changes the size of the messages
reported up top so that the dialog box has to change size over and
over again, you still end up in the deadlock situation!

services/gam/src/lib.rs

@@ -83,6 +83,9 @@ impl Gam {
     /// the actual screen update is allowed
     pub fn post_textview(&self, tv: &mut TextView) -> Result<(), xous::Error> {
         tv.set_op(TextOp::Render);
+        // force the clip_rect to none, in case a stale value from a previous bounds computation was hanging out
+        // the bounds should /always/ come from the GAM canvas when doing a "live fire" redraw
+        tv.clip_rect = None;
         let mut buf = Buffer::into_buf(tv.clone()).or(Err(xous::Error::InternalError))?;
         buf.lend_mut(self.conn, Opcode::RenderTextView.to_u32().unwrap()).or(Err(xous::Error::InternalError))?;
 
@@ -99,6 +102,9 @@ impl Gam {
         tv.set_op(TextOp::Nop);
         Ok(())
     }
+    /// Bounds computation does no checks on security since it's a non-drawing operation. While normal drawing always
+    /// takes the bounds from the canvas, the caller can specify a clip_rect in this tv, instead of drawing the
+    /// clip_rect from the Canvas associated with the tv.
     pub fn bounds_compute_textview(&self, tv: &mut TextView) -> Result<(), xous::Error> {
         tv.set_op(TextOp::ComputeBounds);
         let mut buf = Buffer::into_buf(tv.clone()).or(Err(xous::Error::InternalError))?;

services/gam/src/main.rs

@@ -432,6 +432,7 @@ impl ContextManager {
             }
             // run the defacement before we redraw all the canvases
             deface(gfx, &self.trng, canvases);
+            log::trace!("activate redraw");
             self.redraw().expect("couldn't redraw the currently focused app");
         }
     }
@@ -448,6 +449,7 @@ impl ContextManager {
             for maybe_context in self.contexts.iter() {
                 if let Some(context) = maybe_context {
                     if token == context.app_token {
+                        log::trace!("redraw msg to {}, id {}", context.listener, context.redraw_id);
                         return xous::send_message(context.listener,
                             xous::Message::new_scalar(context.redraw_id as usize, 0, 0, 0, 0)
                         ).map(|_| ())
@@ -716,6 +718,7 @@ fn xmain() -> ! {
                             // we keep this here because it's a fail-safe in case prior routines missed an edge case. shoot out a warning noting the issue.
                             log::warn!("canvases were not defaced in order. running a defacement, but this could result in drawing optimizations failing.");
                             // try to redraw the trusted foreground apps after a defacement
+                            log::trace!("deface redraw");
                             context_mgr.redraw().expect("couldn't redraw after defacement");
                         }
                         log::trace!("flushing...");
@@ -733,7 +736,7 @@ fn xmain() -> ! {
                 log::trace!("rendertextview {:?}", tv);
                 match tv.get_op() {
                     TextOp::Nop => (),
-                    TextOp::Render | TextOp::ComputeBounds => {
+                    TextOp::Render => {
                         if tv.invert & tv.token.is_some() {
                             // an inverted text can only be made by secure processes. check that it has a valid token.
                             if !context_mgr.is_token_valid(tv.token.unwrap()) {
@@ -743,11 +746,7 @@ fn xmain() -> ! {
                         }
 
                         log::trace!("render request for {:?}", tv);
-                        if tv.get_op() == TextOp::ComputeBounds {
-                            tv.set_dry_run(true);
-                        } else {
-                            tv.set_dry_run(false);
-                        }
+                        tv.set_dry_run(false);
 
                         if let Some(canvas) = canvases.get_mut(&tv.get_canvas_gid()) {
                             // if we're requesting inverted text, this better be a "trusted canvas"
@@ -758,12 +757,9 @@ fn xmain() -> ! {
                                 continue;
                             }
                             // first, figure out if we should even be drawing to this canvas.
-                            if canvas.is_drawable() || tv.dry_run() { // dry runs should not move any pixels so they are OK to go through in any case
+                            if canvas.is_drawable() { // dry runs should not move any pixels so they are OK to go through in any case
                                 // set the clip rectangle according to the canvas' location
-                                let mut base_clip_rect = canvas.clip_rect();
-                                if tv.dry_run() {
-                                    base_clip_rect.normalize();
-                                }
+                                let base_clip_rect = canvas.clip_rect();
                                 tv.clip_rect = Some(base_clip_rect.into());
 
                                 // you have to clone the tv object, because if you don't the same block of
@@ -774,17 +770,12 @@ fn xmain() -> ! {
                                 // issue the draw command
                                 gfx.draw_textview(&mut tv_clone).expect("text view draw could not complete.");
                                 // copy back the fields that we want to be mutable
-                                if tv.dry_run() {
-                                    log::trace!("got computed cursor of {:?}, bounds {:?}", tv_clone.cursor, tv_clone.bounds_computed);
-                                }
                                 tv.cursor = tv_clone.cursor;
                                 tv.bounds_computed = tv_clone.bounds_computed;
 
                                 let ret = api::Return::RenderReturn(tv);
                                 buffer.replace(ret).unwrap();
-                                if !tv.dry_run() {
-                                    canvas.do_drawn().expect("couldn't set canvas to drawn");
-                                }
+                                canvas.do_drawn().expect("couldn't set canvas to drawn");
                             } else {
                                 log::debug!("attempt to draw TextView on non-drawable canvas. Not fatal, but request ignored. {:?}", tv);
                                 let ret = api::Return::NotCurrentlyDrawable;
@@ -795,6 +786,34 @@ fn xmain() -> ! {
                             // silently fail if a bogus Gid is given???
                         }
                     },
+                    TextOp::ComputeBounds => {
+                        log::trace!("render request for {:?}", tv);
+                        tv.set_dry_run(true);
+
+                        if tv.clip_rect.is_none() {
+                            // fill in the clip rect from the canvas
+                            if let Some(canvas) = canvases.get_mut(&tv.get_canvas_gid()) {
+                                // set the clip rectangle according to the canvas' location
+                                let mut base_clip_rect = canvas.clip_rect();
+                                base_clip_rect.normalize();
+                                tv.clip_rect = Some(base_clip_rect.into());
+                            } else {
+                                info!("bogus GID {:?} in TextView {}, not doing anything in response to draw request.", tv.get_canvas_gid(), tv.text);
+                                // silently fail if a bogus Gid is given???
+                                continue;
+                            }
+                        }
+                        let mut tv_clone = tv.clone();
+                        // issue the draw command
+                        gfx.draw_textview(&mut tv_clone).expect("text view draw could not complete.");
+                        // copy back the fields that we want to be mutable
+                        log::trace!("got computed cursor of {:?}, bounds {:?}", tv_clone.cursor, tv_clone.bounds_computed);
+                        tv.cursor = tv_clone.cursor;
+                        tv.bounds_computed = tv_clone.bounds_computed;
+
+                        let ret = api::Return::RenderReturn(tv);
+                        buffer.replace(ret).unwrap();
+                    }
                 };
             }
             Some(Opcode::SetCanvasBounds) => {
@@ -811,6 +830,7 @@ fn xmain() -> ! {
                     // recompute the canvas orders based on the new layout
                     let recomp_canvases = recompute_canvases(&canvases, Rectangle::new(Point::new(0, 0), screensize));
                     canvases = recomp_canvases;
+                    log::trace!("canvas bounds redraw");
                     context_mgr.redraw().expect("can't redraw after new canvas bounds");
                 }
                 cb.granted = granted;

services/gam/src/modal.rs

@@ -1094,14 +1094,6 @@ pub enum ModalOpcode { // if changes are made here, also update MenuOpcode
 fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str>, bot_text: Option<&str>, style: GlyphStyle) {
     // we need to set a "max" size to our modal box, so that the text computations don't fail later on
     let current_bounds = modal.gam.get_canvas_bounds(modal.canvas).expect("couldn't get current bounds");
-    let mut new_bounds = SetCanvasBoundsRequest {
-        requested: Point::new(current_bounds.x, crate::api::MODAL_Y_MAX),
-        granted: None,
-        token_type: TokenType::App,
-        token: modal.authtoken,
-    };
-    log::debug!("applying recomputed bounds of {:?}", new_bounds);
-    modal.gam.set_canvas_bounds_request(&mut new_bounds).expect("couldn't call set bounds");
 
     // method:
     //   - we assume the GAM gives us an initial modal with a "maximum" height setting
@@ -1125,6 +1117,9 @@ fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str
         top_tv.margin = Point::new(0, 0,); // all margin already accounted for in the raw bounds of the text drawing
         top_tv.ellipsis = false;
         top_tv.invert = modal.inverted;
+        // specify a clip rect that's the biggest possible allowed. If we don't do this, the current canvas
+        // bounds are used, and the operation will fail if the text has to get bigger.
+        top_tv.clip_rect = Some(Rectangle::new(Point::new(0, 0), Point::new(current_bounds.x, crate::api::MODAL_Y_MAX)));
         write!(top_tv.text, "{}", top_str).unwrap();
 
         log::trace!("posting top tv: {:?}", top_tv);
@@ -1157,6 +1152,9 @@ fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str
         bot_tv.margin = Point::new(0, 0,); // all margin already accounted for in the raw bounds of the text drawing
         bot_tv.ellipsis = false;
         bot_tv.invert = modal.inverted;
+        // specify a clip rect that's the biggest possible allowed. If we don't do this, the current canvas
+        // bounds are used, and the operation will fail if the text has to get bigger.
+        bot_tv.clip_rect = Some(Rectangle::new(Point::new(0, 0), Point::new(current_bounds.x, crate::api::MODAL_Y_MAX)));
         write!(bot_tv.text, "{}", bot_str).unwrap();
 
         log::trace!("posting bot tv: {:?}", bot_tv);
@@ -1179,8 +1177,12 @@ fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str
         token_type: TokenType::App,
         token: modal.authtoken,
     };
-    log::debug!("applying recomputed bounds of {:?}", new_bounds);
-    modal.gam.set_canvas_bounds_request(&mut new_bounds).expect("couldn't call set bounds");
+    // don't send the request if there is no change in the size of things. This is because the request is expensive -- it will
+    // result in a redraw of everything, plus defacement, etc.
+    if new_bounds.requested != current_bounds {
+        log::debug!("applying recomputed bounds of {:?}", new_bounds);
+        modal.gam.set_canvas_bounds_request(&mut new_bounds).expect("couldn't call set bounds");
+    }
 }
 
 impl<'a> Modal<'a> {
@@ -1268,7 +1270,7 @@ impl<'a> Modal<'a> {
         xous::create_thread_3(crate::forwarding_thread, addr, size, offset).expect("couldn't spawn a helper thread");
     }
 
-    pub fn redraw(&self) {
+    pub fn redraw(&mut self) {
         log::debug!("modal redraw");
         let canvas_size = self.gam.get_canvas_bounds(self.canvas).unwrap();
         let do_redraw = self.top_dirty || self.bot_dirty;
@@ -1454,6 +1456,15 @@ impl<'a, 'b> ProgressBar<'a, 'b> {
     pub fn update_text(&mut self, text: &str) {
         self.modal.modify(None, Some(text), false, None, false, None);
     }
+    /// There is a significant caveat for performance/stability for this routine.
+    /// This works well for a modal that is both managed and owned by the same thread so long as
+    /// one is not resizing the top or bottom text within the modal while changing the progress bar
+    /// However, if one triggers a redraw/resize of the text, then the system needs to re-run the
+    /// defacement computations on the underlying canvases. If one or two calls happen, this might
+    /// not be a big deal, but because the redraw requests that are looped back from the GAM as a
+    /// result of the defacement operation cannot be processed until the thread managing the progress
+    /// bar has finished and returned control to the main loop, redraw requests will eventually fill
+    /// up the server queue and cause a deadlock situation.
     fn update_ui(&mut self, new_percent: u32) {
         if new_percent != self.current_progress_percent {
             log::debug!("progress: {}", new_percent);

services/root-keys/src/implementation.rs

@@ -752,7 +752,17 @@ impl<'a> RootKeys {
         rootkeys_modal.activate();
 
         xous::yield_slice(); // give some time to the GAM to render
-        // capture the progress bar elements in a convenience structure
+
+        // Capture the progress bar elements in a convenience structure.
+        // NOTE: This is documented in the structure itself, but, it's worth repeating here:
+        // this routine only works because we don't resize the rootkeys_modal box as we
+        // advance progress. This allows us to do a local-only redraw without triggering
+        // a global GAM redraw operation. If we were to resize the dialog box, this would
+        // trigger the defacing algorithm to send back "redraw" messages into the main
+        // loop's queue. However, because the main loop is currently stuck running the code
+        // in this routine, the "redraw" messages never get serviced (even if they are
+        // effectively NOPs), and eventually, these messages would fill up the queue and can cause
+        // the system to deadlock once the queue is full.
         let mut pb = ProgressBar::new(rootkeys_modal, &mut progress_action);
 
         // kick the progress bar to indicate we've entered the routine

services/root-keys/src/main.rs

@@ -97,6 +97,9 @@ mod implementation {
         pub fn verify_gateware_self_signature(&mut self) -> bool {
             true
         }
+        pub fn test(&mut self, rootkeys_modal: &mut Modal, main_cid: xous::CID) -> Result<(), RootkeyResult> {
+            Ok(())
+        }
     }
 }
 
@@ -106,7 +109,7 @@ fn xmain() -> ! {
     use crate::implementation::RootKeys;
 
     log_server::init_wait().unwrap();
-    log::set_max_level(log::LevelFilter::Debug);
+    log::set_max_level(log::LevelFilter::Info);
     log::info!("my PID is {}", xous::process::id());
 
     let xns = xous_names::XousNames::new().unwrap();
@@ -208,7 +211,7 @@ fn xmain() -> ! {
 
             // UX flow opcodes
             Some(Opcode::UxTryInitKeys) => msg_scalar_unpack!(msg, _, _, _, _, {
-                if true { // short-circuit for testing subroutines
+                if false { // short-circuit for testing subroutines
                     let _success = keys.test(&mut rootkeys_modal, main_cid);
 
                     keys.finish_key_init();