From 757d643f2d2c4d562e7f9161a81171c2ede4a4ac Mon Sep 17 00:00:00 2001 From: tworrall Date: Mon, 25 Nov 2013 10:51:25 -0500 Subject: [PATCH 1/3] fix cross browser console issue --- webapp/web/js/vitroUtils.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/webapp/web/js/vitroUtils.js b/webapp/web/js/vitroUtils.js index e910efa74..10e5f564b 100644 --- a/webapp/web/js/vitroUtils.js +++ b/webapp/web/js/vitroUtils.js @@ -13,20 +13,16 @@ $(document).ready(function(){ jQuery('section#flash-message').css('display', 'none').fadeIn(1500); ///////////////////////////// - // Home search fiter + // Home search filter // Toggle filter select list var $searchFilterList = $('#filter-search-nav'); var $isFilterOpen = false; - console.log("Filter is open = " + $isFilterOpen); - $('a.filter-search').click(function(e) { e.preventDefault(); if (!$isFilterOpen) { - console.log("Filter is closed = " + $isFilterOpen); - //Change button filter state to selected //$(this).css('background','url(../../themes/vivo-cornell/images/filteredSearchActive.gif) no-repeat right top'); $(this).removeClass('filter-default'); @@ -37,7 +33,6 @@ $(document).ready(function(){ $isFilterOpen = true; - console.log("open"); } else { //Change button filter state to default //$('a.filter-search').css('background','url(../../themes/vivo-cornell/images/filteredSearch.gif) no-repeat right top'); @@ -49,7 +44,6 @@ $(document).ready(function(){ $isFilterOpen = false; - console.log("closed"); } }); @@ -63,7 +57,6 @@ $(document).ready(function(){ //Selected filter feedback $('.search-filter-selected').text(''); $('input[name="classgroup"]').val(''); - console.log("ALL"); } else { $('.search-filter-selected').text($(this).text()).fadeIn('slow'); @@ -110,6 +103,5 @@ $(document).ready(function(){ } - console.log("HIDE input value ") ; }); }); From 0b43a4409715cd0f7a549ae69c8835c3302b35b2 Mon Sep 17 00:00:00 2001 From: j2blake Date: Mon, 25 Nov 2013 11:48:11 -0500 Subject: [PATCH 2/3] VIVO-522 fix memory leak in FreemarkerConfigurationImpl --- .../config/FreemarkerConfigurationImpl.java | 47 ++++++++++++++----- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java index 617a09f6c..7d2c05c9d 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java +++ b/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java @@ -3,6 +3,7 @@ package edu.cornell.mannlib.vitro.webapp.freemarker.config; import java.io.IOException; +import java.lang.ref.WeakReference; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -35,28 +36,52 @@ import freemarker.template.utility.DeepUnwrap; * Extend the Freemarker Configuration class to include some information that is * particular to the current request. * - * Takes advantage of the fact that each servlet request runs in a separate - * thread. Stores the request-based information in a ThreadLocal. Override any - * methods that should return that information instead of (or in addition to) - * the common info. + * A reference to the current request is not always available to the Freemarker + * Configuration, so we take advantage of the fact that each request runs in a + * separate thread, and store a reference to that request in a ThreadLocal + * object. + * + * Then, we override any methods that should return that request-based + * information instead of (or in addition to) the common info. * * Only the getters are overridden, not the setters. So if you call * setAllSharedVariables(), for example, it will have no effect on the * request-based information. + * + * Notice that the reference to the current request is actually stored through a + * WeakReference. This is because the ThreadLocal will not be cleared when the + * webapp is stopped, so none of the references from that ThreadLocal are + * eligible for garbage collection. If any of those references is an instance of + * a class that is loaded by the webapp, then the webapp ClassLoader is not + * eligible for garbage collection. This would be a huge memory leak. + * + * Thanks to the WeakReference, the request is eligible for garbage collection + * if nothing else refers to it. In theory, this means that the WeakReference + * could return a null, but if the garbage collector has taken the request, then + * who is invoking this object? */ public class FreemarkerConfigurationImpl extends Configuration { private static final Log log = LogFactory .getLog(FreemarkerConfigurationImpl.class); - private final ThreadLocal rbiRef = new ThreadLocal<>(); + private static final String ATTRIBUTE_NAME = RequestBasedInformation.class + .getName(); + + private final ThreadLocal> reqRef = new ThreadLocal<>(); void setRequestInfo(HttpServletRequest req) { - rbiRef.set(new RequestBasedInformation(req, this)); + reqRef.set(new WeakReference<>(req)); + req.setAttribute(ATTRIBUTE_NAME, new RequestBasedInformation(req, this)); + } + + private RequestBasedInformation getRequestInfo() { + HttpServletRequest req = reqRef.get().get(); + return (RequestBasedInformation) req.getAttribute(ATTRIBUTE_NAME); } @Override public Object getCustomAttribute(String name) { - Map attribs = rbiRef.get().getCustomAttributes(); + Map attribs = getRequestInfo().getCustomAttributes(); if (attribs.containsKey(name)) { return attribs.get(name); } else { @@ -66,13 +91,13 @@ public class FreemarkerConfigurationImpl extends Configuration { @Override public String[] getCustomAttributeNames() { - Set rbiNames = rbiRef.get().getCustomAttributes().keySet(); + Set rbiNames = getRequestInfo().getCustomAttributes().keySet(); return joinNames(rbiNames, super.getCustomAttributeNames()); } @Override public TemplateModel getSharedVariable(String name) { - Map vars = rbiRef.get().getSharedVariables(); + Map vars = getRequestInfo().getSharedVariables(); if (vars.containsKey(name)) { return vars.get(name); } else { @@ -82,7 +107,7 @@ public class FreemarkerConfigurationImpl extends Configuration { @Override public Set getSharedVariableNames() { - Set rbiNames = rbiRef.get().getSharedVariables().keySet(); + Set rbiNames = getRequestInfo().getSharedVariables().keySet(); @SuppressWarnings("unchecked") Set superNames = super.getSharedVariableNames(); @@ -94,7 +119,7 @@ public class FreemarkerConfigurationImpl extends Configuration { @Override public Locale getLocale() { - return rbiRef.get().getReq().getLocale(); + return getRequestInfo().getReq().getLocale(); } private String[] joinNames(Set nameSet, String[] nameArray) { From 70298026ea242c2190198f6927235a1a08233468 Mon Sep 17 00:00:00 2001 From: j2blake Date: Mon, 25 Nov 2013 12:31:51 -0500 Subject: [PATCH 3/3] Reduce warning messages in unit tests. We get a warning on any attempt to use anything that may be affected by DeveloperSettings, because DeveloperSetting tries to use ConfigurationProperties to find its properties file. We could avoid this by requiring the use of a ConfigurationPropertiesStub, but it seems to make more sense to routinely create a DeveloperSettingsStub on each ServletContextStub. --- .../utils/developer/DeveloperSettings.java | 4 +-- .../developer/DeveloperSettingsStub.java | 33 +++++++++++++++++++ .../javax/servlet/ServletContextStub.java | 7 ++++ 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java index 169491372..f883281f9 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java +++ b/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java @@ -179,7 +179,7 @@ public class DeveloperSettings { // The factory // ---------------------------------------------------------------------- - private static final String ATTRIBUTE_NAME = DeveloperSettings.class + protected static final String ATTRIBUTE_NAME = DeveloperSettings.class .getName(); public static DeveloperSettings getBean(HttpServletRequest req) { @@ -203,7 +203,7 @@ public class DeveloperSettings { private final Map settings = new EnumMap<>(Keys.class); - private DeveloperSettings(ServletContext ctx) { + protected DeveloperSettings(ServletContext ctx) { updateFromFile(ctx); } diff --git a/webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java b/webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java new file mode 100644 index 000000000..6e2871596 --- /dev/null +++ b/webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java @@ -0,0 +1,33 @@ +/* $This file is distributed under the terms of the license in /doc/license.txt$ */ + +package stubs.edu.cornell.mannlib.vitro.webapp.utils.developer; + +import javax.servlet.ServletContext; + +import edu.cornell.mannlib.vitro.webapp.utils.developer.DeveloperSettings; + +/** + * Do everything that a standard DeveloperSettings would do, except loading from + * a properties file. + * + * That way, we don't require ConfigurationProperties to find the Vitro home + * directory, so we don't throw errors if there is no ConfigurationProperties. + */ +public class DeveloperSettingsStub extends DeveloperSettings { + /** + * Factory method. Create the stub and set it into the ServletContext. + */ + public static void set(ServletContext ctx) { + ctx.setAttribute(ATTRIBUTE_NAME, new DeveloperSettingsStub(ctx)); + } + + protected DeveloperSettingsStub(ServletContext ctx) { + super(ctx); + } + + @Override + protected void updateFromFile(ServletContext ctx) { + // Don't bother. + } + +} diff --git a/webapp/test/stubs/javax/servlet/ServletContextStub.java b/webapp/test/stubs/javax/servlet/ServletContextStub.java index 46e592d60..eb1e5eab7 100644 --- a/webapp/test/stubs/javax/servlet/ServletContextStub.java +++ b/webapp/test/stubs/javax/servlet/ServletContextStub.java @@ -21,6 +21,8 @@ import javax.servlet.ServletException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import stubs.edu.cornell.mannlib.vitro.webapp.utils.developer.DeveloperSettingsStub; + /** * A simple stand-in for the {@link ServletContext}, for use in unit tests. */ @@ -36,6 +38,11 @@ public class ServletContextStub implements ServletContext { private final Map mockResources = new HashMap(); private final Map realPaths = new HashMap(); + public ServletContextStub() { + // Assume that unit tests won't want to use Developer mode. + DeveloperSettingsStub.set(this); + } + public void setContextPath(String contextPath) { if (contextPath == null) { throw new NullPointerException("contextPath may not be null.");