From 231737ab9161691b733080687ff4363d0a761aea Mon Sep 17 00:00:00 2001 From: --list Date: Sat, 30 Nov 2024 18:23:00 -0700 Subject: [PATCH] SLING-12516 - Migrate SlingContext to Rhino Context(ContextFactory) API * Replace the deprecated Rhino Context() API with the updated Context(ContextFactory) API. * Align the implementation with the Rhino global static context factory model. * Add test cases to ensure alignment with the Rhino global static context factory model. --- .../javascript/helper/SlingContext.java | 5 ++ .../helper/SlingContextFactory.java | 76 +++++++------------ .../RhinoJavaScriptEngineFactory.java | 35 +++++---- .../RhinoJavaScriptEngineFactoryTest.java | 22 ++++++ 4 files changed, 74 insertions(+), 64 deletions(-) diff --git a/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContext.java b/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContext.java index 3a119c9..89c88af 100644 --- a/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContext.java +++ b/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContext.java @@ -19,6 +19,7 @@ package org.apache.sling.scripting.javascript.helper; import org.mozilla.javascript.Context; +import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.ImporterTopLevel; import org.mozilla.javascript.ScriptableObject; @@ -29,6 +30,10 @@ */ public class SlingContext extends Context { + protected SlingContext(ContextFactory factory) { + super(factory); + } + @Override public ScriptableObject initStandardObjects(ScriptableObject scope, boolean sealed) { ScriptableObject rootScope = super.initStandardObjects(scope, sealed); diff --git a/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContextFactory.java b/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContextFactory.java index 82e2bdd..9d9fcb1 100644 --- a/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContextFactory.java +++ b/src/main/java/org/apache/sling/scripting/javascript/helper/SlingContextFactory.java @@ -18,7 +18,7 @@ */ package org.apache.sling.scripting.javascript.helper; -import java.lang.reflect.Field; +import java.util.concurrent.atomic.AtomicReference; import org.mozilla.javascript.Context; import org.mozilla.javascript.ContextFactory; @@ -45,47 +45,41 @@ public class SlingContextFactory extends ContextFactory { private int languageVersion; - // conditionally setup the global ContextFactory to be ours. If - // a global context factory has already been set, we have lost - // and cannot set this one. - public static void setup(ScopeProvider sp, int languageVersion) { - // TODO what do we do in the other case? debugger won't work - if (!hasExplicitGlobal()) { - initGlobal(new SlingContextFactory( - sp, Context.isValidLanguageVersion(languageVersion) ? languageVersion : Context.VERSION_DEFAULT)); - } - } - - public static void teardown() { - ContextFactory factory = getGlobal(); - if (factory instanceof SlingContextFactory) { - ((SlingContextFactory) factory).dispose(); + private static final AtomicReference singleSlingContextFactoryRef = new AtomicReference<>(); + + /** + * @param sp the scope provider + * @param languageVersion the language version. + * @return the SlingContextFactory instance that has been successfully registered as the custom global context factory of the Rhino Runtime. + */ + public static SlingContextFactory getInstance(ScopeProvider sp, int languageVersion) throws RuntimeException { + if (singleSlingContextFactoryRef.get() == null) { + synchronized (SlingContextFactory.class) { + if (singleSlingContextFactoryRef.get() == null) { + try { + SlingContextFactory factory = new SlingContextFactory(sp, languageVersion); + ContextFactory.initGlobal(factory); + singleSlingContextFactoryRef.set(factory); + } catch (IllegalArgumentException e) { + throw new RuntimeException("Fail to set Sling Context Factory as custom context factory", e); + } catch (IllegalStateException e) { + throw new RuntimeException("Custom context factory already initiated.", e); + } + } + } } + return singleSlingContextFactoryRef.get(); } - // private as instances of this class are only used by setup() private SlingContextFactory(ScopeProvider sp, int languageVersion) { scopeProvider = sp; - this.languageVersion = languageVersion; - } - - private void dispose() { - // ensure the debugger is closed - exitDebugger(); - - // reset the context factory class for future use - ContextFactory newGlobal = new ContextFactory(); - setField(newGlobal, "hasCustomGlobal", Boolean.FALSE); - setField(newGlobal, "global", newGlobal); - setField(newGlobal, "sealed", Boolean.FALSE); - setField(newGlobal, "listeners", null); - setField(newGlobal, "disabledListening", Boolean.FALSE); - setField(newGlobal, "applicationClassLoader", null); + this.languageVersion = + Context.isValidLanguageVersion(languageVersion) ? languageVersion : Context.VERSION_DEFAULT; } @Override protected Context makeContext() { - Context context = new SlingContext(); + Context context = new SlingContext(this); context.setLanguageVersion(languageVersion); return context; } @@ -139,20 +133,4 @@ public void setDebugging(boolean enable) { public boolean isDebugging() { return debuggerActive; } - - private void setField(Object instance, String fieldName, Object value) { - try { - Field field = instance.getClass().getDeclaredField(fieldName); - if (!field.isAccessible()) { - field.setAccessible(true); - } - field.set(instance, value); - } catch (IllegalArgumentException iae) { - // don't care, but it is strange anyhow - } catch (IllegalAccessException iae) { - // don't care, but it is strange anyhow - } catch (NoSuchFieldException nsfe) { - // don't care, but it is strange anyhow - } - } } diff --git a/src/main/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactory.java b/src/main/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactory.java index f8f8435..545cf9f 100644 --- a/src/main/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactory.java +++ b/src/main/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactory.java @@ -51,7 +51,6 @@ import org.apache.sling.scripting.javascript.wrapper.ScriptableVersion; import org.apache.sling.scripting.javascript.wrapper.ScriptableVersionHistory; import org.mozilla.javascript.Context; -import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.ImporterTopLevel; import org.mozilla.javascript.NativeJavaClass; import org.mozilla.javascript.NativeJavaPackage; @@ -125,7 +124,7 @@ public class RhinoJavaScriptEngineFactory extends AbstractScriptEngineFactory im private int optimizationLevel; - private static final int RHINO_LANGUAGE_VERSION = Context.VERSION_ES6; + public static final int RHINO_LANGUAGE_VERSION = Context.VERSION_ES6; private static final String LANGUAGE_VERSION = "partial ECMAScript 2015 support"; private static final String LANGUAGE_NAME = "ECMAScript"; @@ -135,6 +134,8 @@ public class RhinoJavaScriptEngineFactory extends AbstractScriptEngineFactory im private final Set hostObjectProvider = new HashSet(); + private SlingContextFactory contextFactory; + @Reference private DynamicClassLoaderManager dynamicClassLoaderManager = null; @@ -252,10 +253,7 @@ private void dropRootScope() { // ensure the debugger is closed if the root scope will // be replaced to ensure no references to the old scope // and context remain - ContextFactory contextFactory = ContextFactory.getGlobal(); - if (contextFactory instanceof SlingContextFactory) { - ((SlingContextFactory) contextFactory).exitDebugger(); - } + contextFactory.exitDebugger(); // drop the scope rootScope = null; @@ -308,7 +306,8 @@ protected void activate( wrapFactory = new SlingWrapFactory(); // initialize the Rhino Context Factory - SlingContextFactory.setup(this, RHINO_LANGUAGE_VERSION); + contextFactory = SlingContextFactory.getInstance(this, RHINO_LANGUAGE_VERSION); + contextFactory.setDebugging(debugging); setEngineName(getEngineName() + " (Rhino " + (rhinoVersion != null ? rhinoVersion : "unknown") + ")"); @@ -316,18 +315,26 @@ protected void activate( setMimeTypes(PropertiesUtil.toStringArray(props.get("mimeTypes"))); setNames(PropertiesUtil.toStringArray(props.get("names"))); - final ContextFactory contextFactory = ContextFactory.getGlobal(); - if (contextFactory instanceof SlingContextFactory) { - ((SlingContextFactory) contextFactory).setDebugging(debugging); - } // set the dynamic class loader as the application class loader final DynamicClassLoaderManager dclm = this.dynamicClassLoaderManager; if (dclm != null) { - contextFactory.initApplicationClassLoader(dynamicClassLoaderManager.getDynamicClassLoader()); + if (contextFactory.getApplicationClassLoader() == null) { + contextFactory.initApplicationClassLoader(dynamicClassLoaderManager.getDynamicClassLoader()); + } else { + log.warn("Ignoring multiple dynamic class loader registration attempts."); + } } log.info("Activated with optimization level {}", optimizationLevel); active = true; + } catch (RuntimeException e) { + /** + * Fail the bundle initialization if SlingContextFactory was not registered as the custom context + * provide to prevent incorrect scripting processing results. + */ + log.error( + "Cannot register SlingContexrtFactory as custom global context provider. This could happen if there is another Rhino JavaScript service conflict."); + throw e; } finally { writeLock.unlock(); } @@ -341,10 +348,8 @@ protected void deactivate(ComponentContext context) { // remove the root scope dropRootScope(); - // remove our context factory - SlingContextFactory.teardown(); - // remove references + contextFactory = null; wrapFactory = null; hostObjectProvider.clear(); diff --git a/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactoryTest.java b/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactoryTest.java index d590351..63e63fe 100644 --- a/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactoryTest.java +++ b/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactoryTest.java @@ -24,10 +24,12 @@ import org.apache.sling.commons.classloader.DynamicClassLoaderManager; import org.apache.sling.scripting.api.ScriptCache; +import org.apache.sling.scripting.javascript.helper.SlingContextFactory; import org.apache.sling.testing.mock.osgi.junit5.OsgiContext; import org.apache.sling.testing.mock.osgi.junit5.OsgiContextExtension; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mozilla.javascript.ContextFactory; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -58,4 +60,24 @@ void testRegistrationProperties() { instance.getEngineName() != null && instance.getEngineName().contains("Rhino 1.7.7.1_1"), "Unexpected engine name"); } + + @Test + void testGlobalSlingContextFactory() { + DynamicClassLoaderManager dynamicClassLoaderManager = mock(DynamicClassLoaderManager.class); + when(dynamicClassLoaderManager.getDynamicClassLoader()) + .thenReturn(RhinoJavaScriptEngineFactoryTest.class.getClassLoader()); + context.registerService(DynamicClassLoaderManager.class, dynamicClassLoaderManager); + context.registerService(ScriptCache.class, mock(ScriptCache.class)); + context.registerInjectActivateService(new RhinoJavaScriptEngineFactory()); + RhinoJavaScriptEngineFactory instance = + (RhinoJavaScriptEngineFactory) context.getService(ScriptEngineFactory.class); + assertTrue( + (ContextFactory.getGlobal() instanceof SlingContextFactory), + "SlingContextFactory type should be the global context factory but is not"); + assertTrue( + ContextFactory.getGlobal() + == (ContextFactory) (SlingContextFactory.getInstance( + instance, RhinoJavaScriptEngineFactory.RHINO_LANGUAGE_VERSION)), + "SlingContextFactory instance should always equal to the registered =global context factory but is not"); + } }