Francois Gouget : testbot/web: Explicitly call exit on return from Redirect().

[Alexandre Julliard]

Module: tools
Branch: master
Commit: d1f74feaa53d4d724b4a3e470eea45d478e7987b
URL:    https://source.winehq.org/git/tools.git/?a=commit;h=d1f74feaa53d4d724b4a3e47...

Author: Francois Gouget <fgouget(a)codeweavers.com>
Date:   Thu Mar 10 11:20:21 2022 +0100

testbot/web: Explicitly call exit on return from Redirect().

Having Redirect() call exit() itself is confusing and requires
documenting that it does not return in each call point and/or adding
a defensive exit() call anyway.
Using the exit(Redirect(...)) construct is more explicit, concise and
robust.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
Signed-off-by: Alexandre Julliard <julliard(a)winehq.org>

---

 testbot/lib/ObjectModel/CGI/CollectionBlock.pm | 18 +++++++-----------
 testbot/lib/ObjectModel/CGI/ItemPage.pm        | 10 ++++------
 testbot/lib/ObjectModel/CGI/Page.pm            |  2 +-
 testbot/lib/WineTestBot/CGI/PageBase.pm        |  6 +++---
 testbot/web/JobDetails.pl                      |  8 +++-----
 testbot/web/Login.pl                           |  3 +--
 testbot/web/Submit.pl                          | 13 +++----------
 testbot/web/admin/Log.pl                       |  4 ++--
 testbot/web/admin/SpecialJobs.pl               |  3 +--
 testbot/web/admin/UserDetails.pl               |  9 +++------
 10 files changed, 28 insertions(+), 48 deletions(-)

diff --git a/testbot/lib/ObjectModel/CGI/CollectionBlock.pm b/testbot/lib/ObjectModel/CGI/CollectionBlock.pm
index a5e8187..9027b87 100644
--- a/testbot/lib/ObjectModel/CGI/CollectionBlock.pm
+++ b/testbot/lib/ObjectModel/CGI/CollectionBlock.pm
@@ -539,20 +539,16 @@ sub OnAction($$)
                    "=" . uri_escape($MasterColValues->[$ColIndex]);
       }
     }
-    $self->{EnclosingPage}->Redirect($Target);
-    return 1;
+    exit($self->{EnclosingPage}->Redirect($Target));
   }
-  else
+
+  my $Ok = 1;
+  foreach my $Key (@{$self->{Collection}->GetKeys()})
   {
-    my $Ok = 1;
-    foreach my $Key (@{$self->{Collection}->GetKeys()})
+    if (defined $self->{EnclosingPage}->GetParam($self->SelName($Key)) && $Ok)
     {
-      if (defined($self->{EnclosingPage}->GetParam($self->SelName($Key))) &&
-          $Ok)
-      {
-        my $Item = $self->{Collection}->GetItem($Key);
-        $Ok = $self->CallOnItemAction($Item, $Action);
-      }
+      my $Item = $self->{Collection}->GetItem($Key);
+      $Ok = $self->CallOnItemAction($Item, $Action);
     }
   }
 }
diff --git a/testbot/lib/ObjectModel/CGI/ItemPage.pm b/testbot/lib/ObjectModel/CGI/ItemPage.pm
index 6d94491..9401154 100644
--- a/testbot/lib/ObjectModel/CGI/ItemPage.pm
+++ b/testbot/lib/ObjectModel/CGI/ItemPage.pm
@@ -151,13 +151,11 @@ sub OnAction($$)
   if ($Action eq "OK")
   {
     return !1 if (!$self->Save());
-    $self->RedirectToList();
-    exit;
+    exit($self->RedirectToList());
   }
-  elsif ($Action eq "Cancel")
+  if ($Action eq "Cancel")
   {
-    $self->RedirectToList();
-    exit;
+    exit($self->RedirectToList());
   }
 
   return $self->SUPER::OnAction($Action);
@@ -177,7 +175,7 @@ sub RedirectToList($)
                  "=" . url_escape($MasterColValues->[$ColIndex]);
     }
   }
-  $self->Redirect($Target); # does not return
+  return $self->Redirect($Target);
 }
 
 1;
diff --git a/testbot/lib/ObjectModel/CGI/Page.pm b/testbot/lib/ObjectModel/CGI/Page.pm
index 8abaaaa..b0450d0 100644
--- a/testbot/lib/ObjectModel/CGI/Page.pm
+++ b/testbot/lib/ObjectModel/CGI/Page.pm
@@ -246,7 +246,7 @@ sub Redirect($$)
 {
   my ($self, $Location) = @_;
 
-  $self->{PageBase}->Redirect($self, $Location);
+  return $self->{PageBase}->Redirect($self, $Location);
 }
 
 sub GetCurrentSession($)
diff --git a/testbot/lib/WineTestBot/CGI/PageBase.pm b/testbot/lib/WineTestBot/CGI/PageBase.pm
index 2681ece..322552d 100644
--- a/testbot/lib/WineTestBot/CGI/PageBase.pm
+++ b/testbot/lib/WineTestBot/CGI/PageBase.pm
@@ -56,7 +56,7 @@ sub new($$$$@)
         ! $Session->User->HasRole($RequiredRole))
     {
       my $LoginURL = "/Login.pl?Target=" . uri_escape($ENV{"REQUEST_URI"});
-      $self->Redirect($Page, MakeSecureURL($LoginURL));
+      exit($self->Redirect($Page, MakeSecureURL($LoginURL)));
     }
   }
 
@@ -75,7 +75,7 @@ sub CheckSecurePage($$)
 
   if ($UseSSL && ! SecureConnection())
   {
-    $self->Redirect($Page, MakeSecureURL($ENV{"REQUEST_URI"}));
+    exit($self->Redirect($Page, MakeSecureURL($ENV{"REQUEST_URI"})));
   }
 }
 
@@ -413,7 +413,7 @@ sub Redirect($$$)
   }
   $self->{Request}->headers_out->set("Location", $Location);
   $self->{Request}->status(Apache2::Const::REDIRECT);
-  exit;
+  return 0; # a suitable exit code
 }
 
 sub GetCurrentSession($)
diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl
index 919c7a5..8d93957 100644
--- a/testbot/web/JobDetails.pl
+++ b/testbot/web/JobDetails.pl
@@ -52,7 +52,7 @@ sub _initialize($$$)
   $self->{Job} = CreateJobs()->GetItem($JobId);
   if (!defined $self->{Job})
   {
-    $self->Redirect("/"); # does not return
+    exit($self->Redirect("/"));
   }
   $self->{JobId} = $JobId;
 
@@ -193,8 +193,7 @@ sub OnCancel($)
     return !1;
   }
 
-  $self->Redirect("/JobDetails.pl?Key=" . $self->{JobId}); # does not return
-  exit;
+  exit($self->Redirect("/JobDetails.pl?Key=" . $self->{JobId}));
 }
 
 sub OnRestart($)
@@ -215,8 +214,7 @@ sub OnRestart($)
     return !1;
   }
 
-  $self->Redirect("/JobDetails.pl?Key=" . $self->{JobId}); # does not return
-  exit;
+  exit($self->Redirect("/JobDetails.pl?Key=" . $self->{JobId}));
 }
 
 sub OnAction($$$)
diff --git a/testbot/web/Login.pl b/testbot/web/Login.pl
index d9757ed..ac64fc2 100644
--- a/testbot/web/Login.pl
+++ b/testbot/web/Login.pl
@@ -135,8 +135,7 @@ sub OnLogIn($)
   {
     $Target = "/";
   }
-  $self->Redirect(MakeSecureURL($Target)); # does not return
-  exit;
+  exit($self->Redirect(MakeSecureURL($Target)));
 }
 
 sub OnAction($$)
diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl
index 98d1338..79d1c87 100644
--- a/testbot/web/Submit.pl
+++ b/testbot/web/Submit.pl
@@ -1265,8 +1265,7 @@ sub _SubmitJob($$)
     return undef;
   }
 
-  $self->Redirect("/JobDetails.pl?Key=". $NewJob->GetKey()); # does not return
-  exit;
+  exit($self->Redirect("/JobDetails.pl?Key=". $NewJob->GetKey()));
 }
 
 sub OnSubmit($)
@@ -1323,14 +1322,8 @@ sub OnOK($)
 {
   my ($self) = @_;
 
-  if (defined $self->{JobKey})
-  {
-    $self->Redirect("/JobDetails.pl?Key=$self->{JobKey}"); # does not return
-  }
-  else
-  {
-    $self->Redirect("/"); # does not return
-  }
+  my $Target = defined $self->{JobKey} ? "/JobDetails.pl?Key=$self->{JobKey}" : "/";
+  exit($self->Redirect($Target));
 }
 
 sub OnAction($$)
diff --git a/testbot/web/admin/Log.pl b/testbot/web/admin/Log.pl
index cb253bd..456e73f 100644
--- a/testbot/web/admin/Log.pl
+++ b/testbot/web/admin/Log.pl
@@ -71,8 +71,8 @@ sub GetActions($)
 sub OnDownload($)
 {
   my ($self) = @_;
-  $self->Redirect("/admin/SendLog.pl?Hours=". $self->GetParam("Hours")); # does not return
-  exit;
+
+  exit($self->Redirect("/admin/SendLog.pl?Hours=". $self->GetParam("Hours")));
 }
 
 sub OnAction($$)
diff --git a/testbot/web/admin/SpecialJobs.pl b/testbot/web/admin/SpecialJobs.pl
index 534336c..02d9f8e 100644
--- a/testbot/web/admin/SpecialJobs.pl
+++ b/testbot/web/admin/SpecialJobs.pl
@@ -237,8 +237,7 @@ sub OnSubmit($)
     return undef;
   }
 
-  $self->Redirect("/"); # does not return
-  exit;
+  exit($self->Redirect("/"));
 }
 
 sub OnAction($$)
diff --git a/testbot/web/admin/UserDetails.pl b/testbot/web/admin/UserDetails.pl
index 2cf0f59..8c26930 100644
--- a/testbot/web/admin/UserDetails.pl
+++ b/testbot/web/admin/UserDetails.pl
@@ -74,8 +74,7 @@ sub OnApprove($)
   return !1 if (!$self->Save());
   $self->{ErrMessage} = $self->{Item}->Approve();
   return !1 if (defined $self->{ErrMessage});
-  $self->RedirectToList(); # does not return
-  exit;
+  exit($self->RedirectToList());
 }
 
 sub OnReject($)
@@ -87,8 +86,7 @@ sub OnReject($)
   return !1 if (defined $self->{ErrMessage});
   # Forcefully log out that user by deleting his web sessions
   DeleteSessions($self->{Item});
-  $self->RedirectToList(); # does not return
-  exit;
+  exit($self->RedirectToList());
 }
 
 sub OnOK($)
@@ -101,8 +99,7 @@ sub OnOK($)
     # Forcefully log out that user by deleting his web sessions
     DeleteSessions($self->{Item});
   }
-  $self->RedirectToList(); # does not return
-  exit;
+  exit($self->RedirectToList());
 }
 
 sub OnAction($$)