486530 - Handler added to WebAppContext prevents ServletContext initialization

Added warnings for loops and inappropriate handlers.
Used insertHandler in more XML files

3 files changed, 55 insertions, 41 deletions

diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java
index c5ffbd382c..3b23ec4f3d 100644
--- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java
+++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java
@@ -66,6 +66,8 @@ import org.eclipse.jetty.util.DeprecationWarning;
 import org.eclipse.jetty.util.annotation.ManagedAttribute;
 import org.eclipse.jetty.util.annotation.ManagedObject;
 import org.eclipse.jetty.util.component.LifeCycle;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 
 /** 
  * Servlet Context.
@@ -83,6 +85,8 @@ import org.eclipse.jetty.util.component.LifeCycle;
 @ManagedObject("Servlet Context Handler")
 public class ServletContextHandler extends ContextHandler
 {
+    private static final Logger LOG = Log.getLogger(ServletContextHandler.class);
+    
     public final static int SESSIONS=1;
     public final static int SECURITY=2;
     public final static int GZIP=4;
@@ -169,7 +173,21 @@ public class ServletContextHandler extends ContextHandler
         
         if (errorHandler!=null)
             setErrorHandler(errorHandler);
+    }
+    
+    @Override
+    public void setHandler(Handler handler)
+    {
+        LOG.warn("ServletContextHandler.setHandler should not be called directly. Use insertHandler or setSessionHandler etc.");
+        super.setHandler(handler);
+    }
 
+    private void doSetHandler(HandlerWrapper wrapper, Handler handler)
+    {
+        if (wrapper==this)
+            super.setHandler(handler);
+        else
+            wrapper.setHandler(handler);
     }
     
     /* ------------------------------------------------------------ */
@@ -189,12 +207,7 @@ public class ServletContextHandler extends ContextHandler
                 handler=(HandlerWrapper)handler.getHandler();
             
             if (handler.getHandler()!=_sessionHandler)
-            {
-                if (handler== this)
-                    super.setHandler(_sessionHandler);
-                else
-                    handler.setHandler(_sessionHandler);
-            }
+                doSetHandler(handler,_sessionHandler);
             handler=_sessionHandler;
         }
         
@@ -208,12 +221,7 @@ public class ServletContextHandler extends ContextHandler
                 handler=(HandlerWrapper)handler.getHandler();
 
             if (handler.getHandler()!=_securityHandler)
-            {
-                if (handler== this)
-                    super.setHandler(_securityHandler);
-                else
-                    handler.setHandler(_securityHandler);
-            }
+                doSetHandler(handler,_securityHandler);
             handler=_securityHandler;
         }
 
@@ -226,12 +234,7 @@ public class ServletContextHandler extends ContextHandler
                 handler=(HandlerWrapper)handler.getHandler();
 
             if (handler.getHandler()!=_gzipHandler)
-            {
-                if (handler== this)
-                    super.setHandler(_gzipHandler);
-                else
-                    handler.setHandler(_gzipHandler);
-            }
+                doSetHandler(handler,_gzipHandler);
             handler=_gzipHandler;
         }
 
@@ -244,12 +247,7 @@ public class ServletContextHandler extends ContextHandler
                 handler=(HandlerWrapper)handler.getHandler();
 
             if (handler.getHandler()!=_servletHandler)
-            {
-                if (handler== this)
-                    super.setHandler(_servletHandler);
-                else
-                    handler.setHandler(_servletHandler);
-            }
+                doSetHandler(handler,_servletHandler);
             handler=_servletHandler;
         }
         
@@ -554,7 +552,7 @@ public class ServletContextHandler extends ContextHandler
         {
             if (wrapper.getHandler()==handler)
             {
-                wrapper.setHandler(replace);
+                doSetHandler(wrapper,replace);
                 return true;
             }
             
@@ -664,20 +662,38 @@ public class ServletContextHandler extends ContextHandler
      */
     public void insertHandler(HandlerWrapper handler)
     {
-        HandlerWrapper h=this;
-        
-        // Skip any injected handlers
-        while (h.getHandler() instanceof HandlerWrapper)
-        {
-            HandlerWrapper wrapper = (HandlerWrapper)h.getHandler();
-            if (wrapper instanceof SessionHandler ||
-                wrapper instanceof SecurityHandler ||
-                wrapper instanceof ServletHandler)
-                break;
-            h=wrapper;
+        if (handler instanceof SessionHandler)
+            setSessionHandler((SessionHandler)handler);
+        else if (handler instanceof SecurityHandler)
+            setSecurityHandler((SecurityHandler)handler);
+        else if (handler instanceof GzipHandler)
+            setGzipHandler((GzipHandler)handler);
+        else if (handler instanceof ServletHandler)
+            setServletHandler((ServletHandler)handler);
+        else
+        {
+            HandlerWrapper tail = handler;
+            while(tail.getHandler() instanceof HandlerWrapper)
+                tail=(HandlerWrapper)tail.getHandler();
+            if (tail.getHandler()!=null)
+                throw new IllegalArgumentException("bad tail of inserted wrapper chain");
+            
+            // Skip any injected handlers
+            HandlerWrapper h=this;
+            while (h.getHandler() instanceof HandlerWrapper)
+            {
+                HandlerWrapper wrapper = (HandlerWrapper)h.getHandler();
+                if (wrapper instanceof SessionHandler ||
+                        wrapper instanceof SecurityHandler ||
+                        wrapper instanceof ServletHandler)
+                    break;
+                h=wrapper;
+            }
+            
+            Handler next=h.getHandler();
+            doSetHandler(h,handler);
+            doSetHandler(tail,next);
         }
-        
-        h.setHandler(handler);
         relinkHandlers();
     }
     
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java
index 4be7ff073c..99ca52f754 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java
@@ -81,7 +81,6 @@ public class GzipHandlerTest
         
         _server.setHandler(gzipHandler);
         gzipHandler.setHandler(context);
-        context.setHandler(servlets);
         servlets.addServletWithMapping(TestServlet.class,"/content");
         servlets.addServletWithMapping(ForwardServlet.class,"/forward");
         servlets.addServletWithMapping(IncludeServlet.class,"/include");
@@ -91,7 +90,6 @@ public class GzipHandlerTest
     
     public static class TestServlet extends HttpServlet
     {
-
         @Override
         protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException
         {
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java
index f088ffd3fc..2b2c671f96 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java
@@ -26,6 +26,7 @@ import java.util.EnumSet;
 
 import javax.servlet.DispatcherType;
 
+import org.eclipse.jetty.server.handler.HandlerWrapper;
 import org.eclipse.jetty.servlet.BaseHolder.Source;
 import org.junit.Before;
 import org.junit.Test;
@@ -428,5 +429,4 @@ public class ServletHandlerTest
     }
   
     
-    
 }