345547 - shutdownhtml will not work under mod_perl

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

Because Bugzilla.pm is only loaded once for every httpd child, the shutdownhtml check will only happen once in each child. Also, the deleting of tainted $ENV vars only happens once in each child.

Comment 1

[Max Kanat-Alexander]

Attachment: Work In Progress

The patch will look something like this.

We also have to register init_page as a handler, inside of the startup file. That may make it hard to run non-Bugzilla code on the same Apache server, though...we'll see.

Comment 2

[Max Kanat-Alexander]

Attachment: v1

This patch re-architects Bugzilla.pm so that when the time comes, I can make shutdownhtml work inside of mod_perl.

Comment 3

[Max Kanat-Alexander]

Okay, now that mod_perl.pl has been checked-in, I have code that fixes the original description of this bug. It's more complex, but it's all done.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Okay, so this patch is considerably more involved that the previous one, and requires some explanation. I suspect justdave will already know some of this, but others may not, and the reasons for some of my decisions should be recorded here:

Okay, so first of all, you have to understand the phases that mod_perl goes through when it handles a URL:

http://perl.apache.org/docs/2.0/user/handlers/http.html#HTTP_Request_Cycle_Phases

The only ones we care about for this patch are ResponseHandler and CleanupHandler.

So here's what there is to know about this patch:

* I moved Bugzilla.pm's init code into a function called init_page. Modules
  are only loaded *once* per httpd child under mod_perl, so having the
  code outside of a function doesn't work.

* Nothing can call request_cache during mod_perl.pl, or during compilation.
  So, _cleanup has to not run if we're under mod_perl.

* One thing to know is that Apache::RequestUtil->request only works during
  the Response phase. So if anything calls request_cache(), it has to happen
  during the ResponseHandler. That's why init_page happens inside of our own
  RepsonseHandler, instead of in a FixupHandler or an InitHandler.

* I've created two handlers, a ResponseHandler to do init_page, and
  a CleanupHandler to make our cleanup code simpler.

* I've moved all of the Apache configuration into mod_perl.pl. This makes
  things a lot easier for people who want to set up a mod_perl installation.
  It also gives us greater control over the Apache configuration when
  we're running under mod_perl.

* These are the only two lines people now need to put in their config for
  running a mod_perl installation:

PerlSwitches -I/var/www/html/mod_perl -w -T
PerlConfigRequire /var/www/html/mod_perl/mod_perl.pl

  It's very important that that's PerlConfigRequire and not PerlRequire.

* Putting ": method" on a handler makes it so that mod_perl passes
  $class as the first argument. That's why our ResponseHandler has
  "sub handler : method", so that I can use $class->SUPER.

* Handlers have to be defined as packages, but they're so simple that I
  just defined them inside of mod_perl.pl itself. I think this is the best
  way to go.

* The patch works, I tested it.

Comment 5

[Dave Miller [:justdave]]

(In reply to comment #4)

> * I've moved all of the Apache configuration into mod_perl.pl. This makes
>   things a lot easier for people who want to set up a mod_perl installation.
>   It also gives us greater control over the Apache configuration when
>   we're running under mod_perl.
> 
> * These are the only two lines people now need to put in their config for
>   running a mod_perl installation:
> 
> PerlSwitches -I/var/www/html/mod_perl -w -T
> PerlConfigRequire /var/www/html/mod_perl/mod_perl.pl
> 
>   It's very important that that's PerlConfigRequire and not PerlRequire.

This is way cool.  So Debian and so forth don't have to change the "use lib" in all of the files if they're willing to require mod_perl to install the package. :)  Just put the -I thing pointing at the right place in the Apache config. :)

PerlSwitches and PerlConfigRequire work inside a <VirtualHost> also, which means you *should* be able to do most of this stuff without interfering with non-Bugzilla mod_perl stuff on the same server, with the possible exception of the pnotes stuff.

I notice sub END was already there and you're just adding stuff to it...  but where's that get called from?  All the docs I can find seem to indicate you probably want sub DESTROY there...

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> I notice sub END was already there and you're just adding stuff to it...  but
> where's that get called from?  All the docs I can find seem to indicate you
> probably want sub DESTROY there...

  END doesn't get used under mod_perl--it would only run once, the first time the process loaded the module. If I've used an END block, it's only for normal CGI, not for mod_perl.

Comment 7

[Dave Miller [:justdave]]

right, so it's never been working all this time without mod_perl anyway?  maybe it should be END {} dropping the "sub".

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 234318 [details] [diff] [review]
v2

what you changed looks good to me anyway. :)

Comment 9

[Max Kanat-Alexander]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.48; previous revision: 1.47
done
Checking in mod_perl.pl;
/cvsroot/mozilla/webtools/bugzilla/mod_perl.pl,v  <--  mod_perl.pl
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.52; previous revision: 1.51
done