From 658ec069b089ee9710d383c2ce00a2b1f8c71853 Mon Sep 17 00:00:00 2001 From: Sean Flanigan Date: Thu, 5 Oct 2017 01:31:30 +1000 Subject: [PATCH] Avoid direct dependency on Nashorn classes --- server/pom.xml | 6 -- .../org/zanata/util/CommonMarkRenderer.java | 62 ++++++++++--------- .../zanata/util/CommonMarkRendererTest.java | 20 +++++- 3 files changed, 53 insertions(+), 35 deletions(-) diff --git a/server/pom.xml b/server/pom.xml index 0452a9dd2ea..ac08b11d13b 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -2201,12 +2201,6 @@ - - - - jdk.nashorn.api.* - - diff --git a/server/services/src/main/java/org/zanata/util/CommonMarkRenderer.java b/server/services/src/main/java/org/zanata/util/CommonMarkRenderer.java index b416b484d8e..d06f299f2c2 100644 --- a/server/services/src/main/java/org/zanata/util/CommonMarkRenderer.java +++ b/server/services/src/main/java/org/zanata/util/CommonMarkRenderer.java @@ -21,16 +21,17 @@ package org.zanata.util; import com.google.common.base.Charsets; -import jdk.nashorn.api.scripting.JSObject; import org.apache.commons.io.IOUtils; import javax.inject.Named; import javax.script.Bindings; import javax.script.Compilable; import javax.script.CompiledScript; +import javax.script.ScriptContext; import javax.script.ScriptEngine; import javax.script.ScriptEngineManager; import javax.script.ScriptException; +import javax.script.SimpleBindings; import java.io.IOException; import java.io.Serializable; import java.net.URL; @@ -56,23 +57,17 @@ public class CommonMarkRenderer implements Serializable { "google-caja/" + VER_SANITIZER + "/html-sanitizer-minified.js"; private static final String RESOURCE_NAME = "META-INF/resources/webjars/" + SCRIPT_NAME; + // Share ScriptEngine and CompiledScript across threads, but not Bindings // See http://stackoverflow.com/a/30159424/14379 private static final ScriptEngine engine = - new ScriptEngineManager().getEngineByName("Nashorn"); - private static final CompiledScript functions = compileFunctions(); + new ScriptEngineManager().getEngineByName("js"); + private static final CompiledScript compiledFunctions = + compileFunctions((Compilable) engine, engine.getBindings(ScriptContext.GLOBAL_SCOPE)); private static final ThreadLocal threadBindings = - ThreadLocal.withInitial(() -> { - Bindings bindings = engine.createBindings(); - // libraries like commonmark.js assume the presence of 'window' - bindings.put("window", bindings); - try { - functions.eval(bindings); - return bindings; - } catch (ScriptException e) { - throw new RuntimeException(e); - } - }); +// ThreadLocal.withInitial(engine::createBindings); + ThreadLocal.withInitial(SimpleBindings::new); + static { log.info("Using commonmark.js version {}", VER); log.info("Using Google Caja version {}", VER_SANITIZER); @@ -121,30 +116,41 @@ public String renderToHtmlSafe(String commonMark) { return HtmlUtil.SANITIZER.sanitize(unsafeHtml); } - public String renderToHtmlUnsafe(String commonMark) { - try { - Bindings bindings = threadBindings.get(); - JSObject mdRender = (JSObject) bindings.get("mdRender"); - return (String) mdRender.call(bindings, commonMark); - } catch (Exception e) { - throw new RuntimeException(e); - } - } - - private static CompiledScript compileFunctions() { + private static CompiledScript compileFunctions(Compilable engine, + Bindings globalBindings) { try { // Create a javascript function 'mdRender' which takes CommonMark // as a string and returns a rendered HTML string: String commonMarkScript = IOUtils.toString(getScriptResource(), Charsets.UTF_8); - String functionsScript = commonMarkScript - + "var reader = new commonmark.Parser();var writer = new commonmark.HtmlRenderer();function mdRender(src) { return writer.render(reader.parse(src));};"; - return ((Compilable) engine).compile(functionsScript); + String functionsScript = commonMarkScript + + "var reader = new commonmark.Parser();" + + "var writer = new commonmark.HtmlRenderer();" + + "var parsed = reader.parse(commonMarkText);" + + "writer.render(parsed);"; + // libraries like commonmark.js assume the presence of 'window' + //noinspection CollectionAddedToSelf + globalBindings.put("window", globalBindings); + return engine.compile(functionsScript); } catch (ScriptException | IOException e) { throw new RuntimeException(e); } } + public String renderToHtmlUnsafe(String commonMark) { + // uncomment this if you want to try sharing the scope between threads (just for testing!) +// Bindings bindings = engine.getBindings(ScriptContext.ENGINE_SCOPE); + Bindings bindings = threadBindings.get(); + bindings.put("commonMarkText", commonMark); + try { + return (String) compiledFunctions.eval(bindings); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + bindings.remove("commonMarkText"); + } + } + private static URL getScriptResource() { String resourceName = "/" + RESOURCE_NAME; URL url = CommonMarkRenderer.class.getResource(resourceName); diff --git a/server/services/src/test/java/org/zanata/util/CommonMarkRendererTest.java b/server/services/src/test/java/org/zanata/util/CommonMarkRendererTest.java index 6e88f0c153f..a6794884462 100644 --- a/server/services/src/test/java/org/zanata/util/CommonMarkRendererTest.java +++ b/server/services/src/test/java/org/zanata/util/CommonMarkRendererTest.java @@ -24,6 +24,9 @@ import org.junit.Rule; import org.junit.Test; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.TimeUnit; + import static org.assertj.core.api.Assertions.*; /** @@ -47,7 +50,6 @@ public void testRenderToHtmlSafe() throws Exception { @Test // 10,000 iterations should run in a few seconds // if you reuse the ScriptEngine and CompiledScript correctly -// @Repeat(times = 10_000) public void testRenderToHtmlUnsafe() throws Exception { String source = "This text contains an *unsafe* element."; String expected = "

This text contains an unsafe element.

\n"; @@ -55,6 +57,22 @@ public void testRenderToHtmlUnsafe() throws Exception { assertThat(rendered).isEqualTo(expected); } + @Test + public void testRenderToHtmlMultithreaded() throws Exception { + String source = "This text contains an *unsafe* element."; + String expected = "

This text contains an unsafe element.

\n"; + + // 10,000 iterations should run in a few seconds + // if you reuse the ScriptEngine and CompiledScript correctly + for (int i = 0; i < 100; i++) { + ForkJoinPool.commonPool().execute(() -> { + String rendered = renderer.renderToHtmlUnsafe(source); + assertThat(rendered).isEqualTo(expected); + }); + } + ForkJoinPool.commonPool().awaitQuiescence(20, TimeUnit.SECONDS); + } + // This test fails with commonmark.js 0.18.1 as minified by // jscompress.com (UglifyJS v1): @Test