This is an automated email from the ASF dual-hosted git repository. royteeuwen pushed a commit to branch bugfix/SLING-13207 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-scripting-javascript.git
commit 2a71bddf076caae0d9e549bebb2b4588679982b3 Author: Roy Teeuwen <[email protected]> AuthorDate: Thu May 21 09:26:22 2026 +0200 SLING-13207: Fix stale NativeJavaPackage cache race condition on DynamicClassLoaderManager rebind --- .../internal/RhinoJavaScriptEngineFactory.java | 84 +++++++++- .../javascript/RepositoryScriptingTestBase.java | 23 ++- .../internal/RhinoJavaScriptEngineTest.java | 24 +++ .../javascript/internal/ScriptEngineHelper.java | 50 +----- .../StalePackageCacheRaceConditionTest.java | 186 +++++++++++++++++++++ .../scripting/javascript/io/EspReaderTest.java | 24 --- 6 files changed, 312 insertions(+), 79 deletions(-) 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..4518b3e 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 @@ -67,6 +67,7 @@ import org.osgi.service.component.annotations.Deactivate; import org.osgi.service.component.annotations.Reference; import org.osgi.service.component.annotations.ReferenceCardinality; import org.osgi.service.component.annotations.ReferencePolicy; +import org.osgi.service.component.annotations.ReferencePolicyOption; import org.osgi.service.metatype.annotations.Designate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -135,8 +136,12 @@ public class RhinoJavaScriptEngineFactory extends AbstractScriptEngineFactory im private final Set<RhinoHostObjectProvider> hostObjectProvider = new HashSet<RhinoHostObjectProvider>(); - @Reference - private DynamicClassLoaderManager dynamicClassLoaderManager = null; + @Reference( + policy = ReferencePolicy.DYNAMIC, + policyOption = ReferencePolicyOption.GREEDY, + bind = "bindDynamicClassLoaderManager", + unbind = "unbindDynamicClassLoaderManager") + private volatile DynamicClassLoaderManager dynamicClassLoaderManager = null; @Reference private ScriptCache scriptCache = null; @@ -320,10 +325,15 @@ public class RhinoJavaScriptEngineFactory extends AbstractScriptEngineFactory im 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()); + // Set a stable wrapper as Rhino's application class loader. + // SlingContextFactory.dispose() resets applicationClassLoader to null on every + // deactivate(), so initApplicationClassLoader() can be called cleanly on each + // activation with a fresh IndirectClassLoader tied to this factory instance. + // The IndirectClassLoader delegates to this.dynamicClassLoaderManager at call + // time, so the underlying ClassLoaderFacade can be swapped on bundle restarts + // (via bindDynamicClassLoaderManager) without violating the called-once contract. + if (contextFactory instanceof SlingContextFactory) { + contextFactory.initApplicationClassLoader(new IndirectClassLoader()); } log.info("Activated with optimization level {}", optimizationLevel); @@ -354,6 +364,68 @@ public class RhinoJavaScriptEngineFactory extends AbstractScriptEngineFactory im } } + @SuppressWarnings("unused") + protected void bindDynamicClassLoaderManager(DynamicClassLoaderManager dclm) { + this.dynamicClassLoaderManager = dclm; + // Drop the rootScope so Rhino rebuilds it with a fresh NativeJavaPackage / NativeJavaClass tree. + writeLock.lock(); + try { + dropRootScope(); + } finally { + writeLock.unlock(); + } + } + + @SuppressWarnings("unused") + protected void unbindDynamicClassLoaderManager(DynamicClassLoaderManager dclm) { + if (this.dynamicClassLoaderManager == dclm) { + this.dynamicClassLoaderManager = null; + } + } + + /** + * A class loader that always delegates to the current {@link DynamicClassLoaderManager}. + * + * <p>Rhino's {@code ContextFactory.initApplicationClassLoader()} can only be called once + * per lifetime of the global {@code ContextFactory} instance. {@link SlingContextFactory#dispose()} + * resets {@code applicationClassLoader} to {@code null} on every {@link #deactivate}, so + * a fresh {@code IndirectClassLoader} is registered on each activation. By delegating to + * {@code this.dynamicClassLoaderManager} at call time rather than capturing a fixed + * {@code ClassLoaderFacade}, it remains correct across {@link #bindDynamicClassLoaderManager} + * rebinds (i.e. every OSGi bundle restart) without needing to re-register with Rhino. + * <p> + * The bundle classloader is used as the parent so that Rhino's own classes (loaded by + * {@code org.mozilla.javascript.*} imports in this bundle) can be resolved. Rhino validates + * the registered loader via {@code Kit.testIfCanLoadRhinoClasses(loader)} and rejects loaders + * that cannot resolve its own classes. Application classes are resolved first via the + * {@code DynamicClassLoaderManager}; the parent bundle classloader acts as fallback for + * framework and Rhino classes. + */ + private class IndirectClassLoader extends ClassLoader { + + IndirectClassLoader() { + // Use the bundle classloader as parent so Rhino's Kit.testIfCanLoadRhinoClasses() + // validation passes (it checks that org.mozilla.javascript.* are resolvable). + super(RhinoJavaScriptEngineFactory.class.getClassLoader()); + } + + @Override + public Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException { + // Application classes (e.g. Sling API, user bundles) come from the dynamic + // classloader. Fall back to the parent (bundle CL) for Rhino and OSGi framework + // classes that are not exported into the dynamic classloader's scope. + final DynamicClassLoaderManager dclm = dynamicClassLoaderManager; + if (dclm != null) { + try { + return dclm.getDynamicClassLoader().loadClass(name); + } catch (ClassNotFoundException ignored) { + // fall through to bundle classloader + } + } + return super.loadClass(name, resolve); + } + } + @SuppressWarnings("unused") protected void addHostObjectProvider(RhinoHostObjectProvider provider) { hostObjectProvider.add(provider); diff --git a/src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java b/src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java index e630f28..3665ec8 100644 --- a/src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java +++ b/src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java @@ -22,14 +22,25 @@ import javax.jcr.Node; import javax.jcr.RepositoryException; import javax.naming.NamingException; +import org.apache.sling.commons.classloader.DynamicClassLoaderManager; import org.apache.sling.commons.testing.jcr.RepositoryTestBase; +import org.apache.sling.scripting.api.ScriptCache; +import org.apache.sling.scripting.javascript.internal.RhinoJavaScriptEngineFactory; import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper; +import org.apache.sling.testing.mock.osgi.junit5.OsgiContext; +import org.apache.sling.testing.mock.osgi.junit5.OsgiContextExtension; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.extension.ExtendWith; -/** Base class for tests which need a Repository - * and scripting functionality */ +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** Base class for tests which need a Repository and scripting functionality */ +@ExtendWith(OsgiContextExtension.class) public class RepositoryScriptingTestBase extends RepositoryTestBase { + + private final OsgiContext osgiContext = new OsgiContext(); protected ScriptEngineHelper script; private int counter; @@ -37,7 +48,13 @@ public class RepositoryScriptingTestBase extends RepositoryTestBase { @BeforeEach protected void setUp() throws Exception { super.setUp(); - script = new ScriptEngineHelper(); + DynamicClassLoaderManager dclm = mock(DynamicClassLoaderManager.class); + when(dclm.getDynamicClassLoader()).thenReturn(getClass().getClassLoader()); + osgiContext.registerService(DynamicClassLoaderManager.class, dclm); + osgiContext.registerService(ScriptCache.class, mock(ScriptCache.class)); + RhinoJavaScriptEngineFactory factory = new RhinoJavaScriptEngineFactory(); + osgiContext.registerInjectActivateService(factory); + script = new ScriptEngineHelper(factory.getScriptEngine()); } @Override diff --git a/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineTest.java b/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineTest.java index 577b052..f3dbe92 100644 --- a/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineTest.java +++ b/src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineTest.java @@ -24,9 +24,13 @@ import javax.script.ScriptException; import javax.script.SimpleBindings; import org.apache.sling.api.scripting.LazyBindings; +import org.apache.sling.commons.classloader.DynamicClassLoaderManager; import org.apache.sling.scripting.api.ScriptCache; import org.apache.sling.scripting.javascript.helper.SlingWrapFactory; +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.mockito.Mockito; import org.mozilla.javascript.Context; import org.mozilla.javascript.ImporterTopLevel; @@ -36,11 +40,16 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +@ExtendWith(OsgiContextExtension.class) class RhinoJavaScriptEngineTest { private static ScriptCache scriptCache = Mockito.mock(ScriptCache.class); + private final OsgiContext osgiContext = new OsgiContext(); + @Test void testPreserveScopeBetweenEvals() throws ScriptException { MockRhinoJavaScriptEngineFactory factory = new MockRhinoJavaScriptEngineFactory(); @@ -86,6 +95,21 @@ class RhinoJavaScriptEngineTest { assertEquals(42.0, result); } + @Test + void testNumericExpressionOutput() throws ScriptException { + DynamicClassLoaderManager dclm = mock(DynamicClassLoaderManager.class); + when(dclm.getDynamicClassLoader()).thenReturn(getClass().getClassLoader()); + osgiContext.registerService(DynamicClassLoaderManager.class, dclm); + osgiContext.registerService(ScriptCache.class, mock(ScriptCache.class)); + RhinoJavaScriptEngineFactory factory = new RhinoJavaScriptEngineFactory(); + osgiContext.registerInjectActivateService(factory); + ScriptEngineHelper script = new ScriptEngineHelper(factory.getScriptEngine()); + + assertEquals("1", script.evalToString("out.write( 1 );")); + assertEquals("1", script.evalToString("out.write( \"1\" );")); + assertEquals("1", script.evalToString("out.write( '1' );")); + } + private static class MockRhinoJavaScriptEngineFactory extends RhinoJavaScriptEngineFactory { protected SlingWrapFactory wrapFactory; diff --git a/src/test/java/org/apache/sling/scripting/javascript/internal/ScriptEngineHelper.java b/src/test/java/org/apache/sling/scripting/javascript/internal/ScriptEngineHelper.java index 98fcb62..861e988 100644 --- a/src/test/java/org/apache/sling/scripting/javascript/internal/ScriptEngineHelper.java +++ b/src/test/java/org/apache/sling/scripting/javascript/internal/ScriptEngineHelper.java @@ -31,49 +31,21 @@ import java.io.StringWriter; import java.util.HashMap; import java.util.Map; -import org.apache.sling.commons.testing.osgi.MockBundle; -import org.apache.sling.commons.testing.osgi.MockComponentContext; -import org.apache.sling.scripting.api.ScriptCache; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.MockitoAnnotations; import org.mozilla.javascript.Context; import org.mozilla.javascript.ScriptableObject; import org.mozilla.javascript.Wrapper; -import org.osgi.framework.BundleContext; - -import static org.mockito.Mockito.mock; /** Helpers to run javascript code fragments in tests */ public class ScriptEngineHelper { - private static ScriptEngine engine; - - @Mock - private static ScriptCache scriptCache; - - @Mock - private RhinoJavaScriptEngineFactoryConfiguration factoryConfiguration; - - @InjectMocks - private RhinoJavaScriptEngineFactory factory; + private final ScriptEngine engine; - public ScriptEngineHelper() { - MockitoAnnotations.openMocks(this); + public ScriptEngineHelper(ScriptEngine engine) { + this.engine = engine; } public static class Data extends HashMap<String, Object> {} - private ScriptEngine getEngine() { - if (engine == null) { - synchronized (ScriptEngineHelper.class) { - factory.activate(new RhinoMockComponentContext(), factoryConfiguration); - engine = factory.getScriptEngine(); - } - } - return engine; - } - public String evalToString(String javascriptCode) throws ScriptException { return evalToString(javascriptCode, null); } @@ -103,7 +75,7 @@ public class ScriptEngineHelper { ctx.setBindings(b, ScriptContext.ENGINE_SCOPE); ctx.setWriter(sw); ctx.setErrorWriter(new OutputStreamWriter(System.err)); - Object result = getEngine().eval(javascriptCode, ctx); + Object result = engine.eval(javascriptCode, ctx); if (result instanceof Wrapper) { result = ((Wrapper) result).unwrap(); @@ -120,18 +92,4 @@ public class ScriptEngineHelper { return result; } - - private static class RhinoMockComponentContext extends MockComponentContext { - - private BundleContext bundleContext = mock(BundleContext.class); - - private RhinoMockComponentContext() { - super(new MockBundle(0)); - } - - @Override - public BundleContext getBundleContext() { - return bundleContext; - } - } } diff --git a/src/test/java/org/apache/sling/scripting/javascript/internal/StalePackageCacheRaceConditionTest.java b/src/test/java/org/apache/sling/scripting/javascript/internal/StalePackageCacheRaceConditionTest.java new file mode 100644 index 0000000..0bb58c7 --- /dev/null +++ b/src/test/java/org/apache/sling/scripting/javascript/internal/StalePackageCacheRaceConditionTest.java @@ -0,0 +1,186 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.scripting.javascript.internal; + +import javax.script.ScriptEngineFactory; +import javax.script.ScriptException; + +import org.apache.sling.commons.classloader.DynamicClassLoaderManager; +import org.apache.sling.scripting.api.ScriptCache; +import org.apache.sling.testing.mock.osgi.junit5.OsgiContext; +import org.apache.sling.testing.mock.osgi.junit5.OsgiContextExtension; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Verifies that {@code RhinoJavaScriptEngineFactory} correctly resolves Java classes + * via {@code Packages.x.y.Z} under normal conditions and after a + * {@code DynamicClassLoaderManager} rebind — the event that occurs on every OSGi bundle + * restart. + * + * <h2>Background</h2> + * {@code RhinoJavaScriptEngineFactory} maintains a single shared {@code rootScope} + * ({@code ImporterTopLevel}) reused by every {@link javax.script.ScriptEngine} instance. + * The scope contains a tree of {@code NativeJavaPackage} objects whose resolved class + * references are cached permanently in each package's {@code ScriptableObject} property + * table. + * + * <p>When {@code DynamicClassLoaderManager} is re-registered (triggered by + * {@code DynamicClassLoaderManagerFactory.Activator.bundleChanged()} on any bundle + * restart), {@code bindDynamicClassLoaderManager()} drops the {@code rootScope} so that + * the stale {@code NativeJavaPackage} tree is discarded and all class lookups are retried + * from scratch with the fresh classloader. + */ +@ExtendWith(OsgiContextExtension.class) +class StalePackageCacheRaceConditionTest { + + // --------------------------------------------------------------------------- + // Infrastructure shared across all tests + // --------------------------------------------------------------------------- + + /** + * Class loader whose ability to load a specific class can be toggled at runtime. + * When a class name is blocked, {@code loadClass} throws {@code ClassNotFoundException}, + * simulating the window during which {@code ClassLoaderFacade.isLive()} is false. + * All other names are forwarded to the real parent. + */ + static class SwitchableClassLoader extends ClassLoader { + private volatile String blockedClassName; + + SwitchableClassLoader(ClassLoader parent) { + super(parent); + } + + void block(String className) { + blockedClassName = className; + } + + void unblock() { + blockedClassName = null; + } + + @Override + public Class<?> loadClass(String name) throws ClassNotFoundException { + if (name.equals(blockedClassName)) { + throw new ClassNotFoundException( + "Simulating DynamicClassLoaderManager invalidation during bundle restart: " + name); + } + return super.loadClass(name); + } + } + + private final OsgiContext context = new OsgiContext(); + + private SwitchableClassLoader switchableLoader; + private RhinoJavaScriptEngineFactory rhinoFactory; + private ScriptEngineFactory factory; + + @BeforeEach + void setUp() { + switchableLoader = new SwitchableClassLoader(StalePackageCacheRaceConditionTest.class.getClassLoader()); + + DynamicClassLoaderManager dclm = mock(DynamicClassLoaderManager.class); + when(dclm.getDynamicClassLoader()).thenReturn(switchableLoader); + context.registerService(DynamicClassLoaderManager.class, dclm); + context.registerService(ScriptCache.class, mock(ScriptCache.class)); + + rhinoFactory = new RhinoJavaScriptEngineFactory(); + context.registerInjectActivateService(rhinoFactory); + factory = context.getService(ScriptEngineFactory.class); + } + + // --------------------------------------------------------------------------- + // Helpers + // --------------------------------------------------------------------------- + + private static final String PROBE = "typeof Packages.org.apache.sling.api.resource.ValueMap"; + + private void assertHealthy(String context) throws ScriptException { + assertEquals( + "function", + factory.getScriptEngine().eval(PROBE), + context + ": ValueMap must resolve to NativeJavaClass (typeof === 'function')"); + } + + // --------------------------------------------------------------------------- + // Tests + // --------------------------------------------------------------------------- + + /** + * Under normal conditions the factory resolves {@code ValueMap} to a + * {@code NativeJavaClass} ({@code typeof === "function"}). + */ + @Test + void classResolution_returnsNativeJavaClass_underNormalConditions() throws ScriptException { + assertHealthy("Normal conditions"); + } + + /** + * After a {@code DynamicClassLoaderManager} rebind — which drops and rebuilds the + * shared {@code rootScope} — class resolution is correct even if the previous scope + * was exposed to a classloader failure. + * + * <p>This is the primary regression test for the stale-{@code NativeJavaPackage} bug: + * a classloader failure during a bundle restart window stores a child + * {@code NativeJavaPackage} permanently in the {@code ScriptableObject} property table. + * The rebind discards the entire tree so all lookups are retried from scratch. + */ + @Test + void classResolution_recoversAfterClassloaderFailureAndRebind() throws ScriptException { + // Simulate the bundle restart window: classloader briefly cannot load ValueMap. + switchableLoader.block("org.apache.sling.api.resource.ValueMap"); + factory.getScriptEngine().eval(PROBE); // poisons the current rootScope + + // Bundle restart completes: DynamicClassLoaderManager is re-registered. + // bindDynamicClassLoaderManager() drops the rootScope — same code path as production. + switchableLoader.unblock(); + DynamicClassLoaderManager freshDclm = mock(DynamicClassLoaderManager.class); + when(freshDclm.getDynamicClassLoader()).thenReturn(switchableLoader); + rhinoFactory.bindDynamicClassLoaderManager(freshDclm); + + assertHealthy("After classloader failure and DynamicClassLoaderManager rebind"); + } + + /** + * After a rebind, all engine instances — not just the first one — resolve + * {@code ValueMap} correctly. Verifies that the shared {@code rootScope} is fully + * rebuilt and not partially stale. + */ + @Test + void classResolution_allEnginesHealthyAfterRebind() throws ScriptException { + // Poison then rebind (same sequence as the test above). + switchableLoader.block("org.apache.sling.api.resource.ValueMap"); + factory.getScriptEngine().eval(PROBE); + + switchableLoader.unblock(); + DynamicClassLoaderManager freshDclm = mock(DynamicClassLoaderManager.class); + when(freshDclm.getDynamicClassLoader()).thenReturn(switchableLoader); + rhinoFactory.bindDynamicClassLoaderManager(freshDclm); + + // Multiple engines obtained after the rebind must all resolve correctly. + for (int i = 0; i < 5; i++) { + assertHealthy("Engine #" + i + " after rebind"); + } + } +} diff --git a/src/test/java/org/apache/sling/scripting/javascript/io/EspReaderTest.java b/src/test/java/org/apache/sling/scripting/javascript/io/EspReaderTest.java index d8c9e77..0418e9f 100644 --- a/src/test/java/org/apache/sling/scripting/javascript/io/EspReaderTest.java +++ b/src/test/java/org/apache/sling/scripting/javascript/io/EspReaderTest.java @@ -18,14 +18,11 @@ */ package org.apache.sling.scripting.javascript.io; -import javax.script.ScriptException; - import java.io.IOException; import java.io.Reader; import java.io.StringReader; import java.util.stream.Stream; -import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper; import org.junit.jupiter.api.Named; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -242,27 +239,6 @@ class EspReaderTest { assertEquals(expected, actual); } - /** Test a complete template, using all features */ - @Test - void testNumericExpressionOutput() throws ScriptException { - ScriptEngineHelper script = new ScriptEngineHelper(); - - String input = "out.write( 1 );"; - String actual = script.evalToString(input); - String expected = "1"; - assertEquals(expected, actual); - - input = "out.write( \"1\" );"; - actual = script.evalToString(input); - expected = "1"; - assertEquals(expected, actual); - - input = "out.write( '1' );"; - actual = script.evalToString(input); - expected = "1"; - assertEquals(expected, actual); - } - @Test void testColon() throws IOException { final String input = "currentNode.text:<%= currentNode.text %>";
