From 34aaf6aba1765c0fe94483445cf2702e29be3584 Mon Sep 17 00:00:00 2001 From: danfickle Date: Sat, 19 Dec 2020 23:56:37 +1100 Subject: [PATCH] #467 (related) Prevent CSS import loop + With test. + Cleanup of CSS code. + Removed stylesheet cache. --- .../openhtmltopdf/context/StyleReference.java | 24 ----- .../context/StylesheetFactoryImpl.java | 93 +++++------------- .../openhtmltopdf/css/parser/CSSParser.java | 2 +- .../css/sheet/StylesheetInfo.java | 36 ++++--- .../openhtmltopdf/layout/SharedContext.java | 18 ---- .../extend/XhtmlCssOnlyNamespaceHandler.java | 11 +-- .../com/openhtmltopdf/util/LogMessageId.java | 1 + .../visualtest/expected/css-import-loop.pdf | Bin 0 -> 1092 bytes .../visualtest/html/css-import-loop.html | 14 +++ .../visualtest/html/stylesheets/loop-one.css | 3 + .../visualtest/html/stylesheets/loop-two.css | 3 + .../VisualRegressionTest.java | 5 + .../openhtmltopdf/java2d/Java2DRenderer.java | 5 - .../pdfboxout/PdfBoxRenderer.java | 5 - 14 files changed, 74 insertions(+), 146 deletions(-) create mode 100644 openhtmltopdf-examples/src/main/resources/visualtest/expected/css-import-loop.pdf create mode 100644 openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html create mode 100644 openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css create mode 100644 openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java index 3efdcbe3f..c926abc8b 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java @@ -36,7 +36,6 @@ import com.openhtmltopdf.css.extend.lib.DOMTreeResolver; import com.openhtmltopdf.css.newmatch.CascadedStyle; import com.openhtmltopdf.css.newmatch.PageInfo; -import com.openhtmltopdf.css.newmatch.Selector; import com.openhtmltopdf.css.parser.CSSPrimitiveValue; import com.openhtmltopdf.css.sheet.PropertyDeclaration; import com.openhtmltopdf.css.sheet.Stylesheet; @@ -211,29 +210,6 @@ public PageInfo getPageStyle(String pageName, String pseudoPage) { return _matcher.getPageCascadedStyle(pageName, pseudoPage); } - /** - * Flushes any stylesheet associated with this stylereference (based on the user agent callback) that are in cache. - * Deprecated for now, until we fix caching, use a new StylesheetFactory each run. - */ - @Deprecated - public void flushStyleSheets() { - String uri = _uac.getBaseURL(); - StylesheetInfo info = new StylesheetInfo(); - info.setUri(uri); - info.setOrigin(StylesheetInfo.AUTHOR); - if (_stylesheetFactory.containsStylesheet(uri)) { - _stylesheetFactory.removeCachedStylesheet(uri); - XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.CSS_PARSE_REMOVING_STYLESHEET_URI_FROM_CACHE_BY_REQUEST, uri); - } else { - XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.CSS_PARSE_REQUESTED_REMOVING_STYLESHEET_URI_NOT_IN_CACHE, uri); - } - } - - @Deprecated - public void flushAllStyleSheets() { - _stylesheetFactory.flushCachedStylesheets(); - } - /** * Gets StylesheetInfos for all stylesheets and inline styles associated * with the current document. Default (user agent) stylesheet and the inline diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java index ff1023c9a..2eac7523a 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java @@ -21,6 +21,8 @@ import java.io.IOException; import java.io.Reader; +import java.util.HashMap; +import java.util.Map; import java.util.logging.Level; import com.openhtmltopdf.css.extend.StylesheetFactory; @@ -46,21 +48,19 @@ public class StylesheetFactoryImpl implements StylesheetFactory { */ private UserAgentCallback _userAgentCallback; - private final int _cacheCapacity = 16; + /** + * This may avoid @import loops, ie. one.css includes two.css + * which then includes one.css. + */ + private final Map _seenStylesheetUris = new HashMap<>(); /** - * an LRU cache + * The maximum number of times a stylesheet uri can be link or + * imported before we give up and conclude there is a loop. */ - private final java.util.LinkedHashMap _cache = - new java.util.LinkedHashMap(_cacheCapacity, 0.75f, true) { - private static final long serialVersionUID = 1L; + private static final int MAX_STYLESHEET_INCLUDES = 10; - protected boolean removeEldestEntry(java.util.Map.Entry eldest) { - return size() > _cacheCapacity; - } - }; - - private CSSParser _cssParser; + private final CSSParser _cssParser; public StylesheetFactoryImpl(UserAgentCallback userAgentCallback) { _userAgentCallback = userAgentCallback; @@ -108,74 +108,27 @@ public Ruleset parseStyleDeclaration(int origin, String styleDeclaration) { } /** - * Adds a stylesheet to the factory cache. Will overwrite older entry for - * same key. - * - * @param key Key to use to reference sheet later; must be unique in - * factory. - * @param sheet The sheet to cache. - */ - @Deprecated - public void putStylesheet(String key, Stylesheet sheet) { - _cache.put(key, sheet); - } - - /** - * @param key - * @return true if a Stylesheet with this key has been put in the cache. - * Note that the Stylesheet may be null. - */ - //TODO: work out how to handle caching properly, with cache invalidation - @Deprecated - public boolean containsStylesheet(String key) { - return _cache.containsKey(key); - } - - /** - * Returns a cached sheet by its key; null if no entry for that key. - * - * @param key The key for this sheet; same as key passed to - * putStylesheet(); - * @return The stylesheet - */ - @Deprecated - public Stylesheet getCachedStylesheet(String key) { - return _cache.get(key); - } - - /** - * Removes a cached sheet by its key. - * - * @param key The key for this sheet; same as key passed to - * putStylesheet(); - */ - @Deprecated - public Object removeCachedStylesheet(String key) { - return _cache.remove(key); - } - - @Deprecated - public void flushCachedStylesheets() { - _cache.clear(); - } - - /** - * Returns a cached sheet by its key; loads and caches it if not in cache; + * Returns a sheet by its key * null if not able to load * + * * @param info The StylesheetInfo for this sheet * @return The stylesheet */ - //TODO: this looks a bit odd public Stylesheet getStylesheet(StylesheetInfo info) { XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.LOAD_REQUESTING_STYLESHEET_AT_URI, info.getUri()); - Stylesheet s = getCachedStylesheet(info.getUri()); - if (s == null && !containsStylesheet(info.getUri())) { - s = parse(info); - putStylesheet(info.getUri(), s); + Integer includeCount = _seenStylesheetUris.get(info.getUri()); + + if (includeCount != null && includeCount >= MAX_STYLESHEET_INCLUDES) { + // Probably an import loop. + XRLog.log(Level.SEVERE, LogMessageId.LogMessageId2Param.CSS_PARSE_TOO_MANY_STYLESHEET_IMPORTS, includeCount, info.getUri()); + return null; } - return s; + + _seenStylesheetUris.merge(info.getUri(), 1, (oldV, newV) -> oldV + 1); + + return parse(info); } public void setUserAgentCallback(UserAgentCallback userAgent) { diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java index 7d5958b80..0f0692978 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java @@ -300,7 +300,7 @@ private void import_rule(Stylesheet stylesheet) throws IOException { t, new Token[] { Token.TK_STRING, Token.TK_URI }, getCurrentLine()); } - if (info.getMedia().size() == 0) { + if (info.getMedia().isEmpty()) { info.addMedium("all"); } stylesheet.addImportRule(info); diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java index f4309015c..15e53f9aa 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java @@ -20,6 +20,7 @@ package com.openhtmltopdf.css.sheet; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Locale; @@ -38,12 +39,12 @@ */ public class StylesheetInfo { - private Stylesheet stylesheet = null;//just to be able to attach "dummy" stylesheets. Also might save a lookup if it's already looked up + private Stylesheet stylesheet = null; private String title; private String uri; private int origin = USER_AGENT; private String type; - private List mediaTypes = new ArrayList<>(); + private final List mediaTypes = new ArrayList<>(); private String content; /** Origin of stylesheet - user agent */ @@ -54,15 +55,20 @@ public class StylesheetInfo { /** Origin of stylesheet - author */ public final static int AUTHOR = 2; - /** * @param m a single media identifier * @return true if the stylesheet referenced applies to the medium */ public boolean appliesToMedia(String m) { - return m.toLowerCase(Locale.US).equals("all") || - mediaTypes.contains("all") || mediaTypes.contains(m.toLowerCase(Locale.US)); + if (mediaTypes.contains("all")) { + return true; + } + + String media = m.toLowerCase(Locale.US); + + return media.equals("all") || + mediaTypes.contains(media); } /** @@ -80,18 +86,18 @@ public void setUri(String uri) { * @param media The new media value */ public void setMedia(String media) { - String[] mediaTypes = media.split(","); this.mediaTypes.clear(); - - for (int i = 0; i < mediaTypes.length; i++) { - this.mediaTypes.add(mediaTypes[i].trim().toLowerCase(Locale.US)); + + if (media == null || media.isEmpty()) { + // Common case, no media attribute, applies to all. + this.addMedium("all"); + } else { + Arrays.stream(media.split(",")) + .map(mediaType -> mediaType.trim().toLowerCase(Locale.US)) + .forEach(this::addMedium); } } - - public void setMedia(List mediaTypes) { - this.mediaTypes = mediaTypes; - } - + public void addMedium(String medium) { mediaTypes.add(medium); } @@ -193,7 +199,7 @@ public String getContent() { public void setContent(String content) { this.content = content; } - + public boolean isInline() { return this.content != null; } diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java index a34feb060..f1c9e97cb 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java @@ -129,24 +129,6 @@ public class SharedContext { public SharedContext() { } - - /** - * Constructor for the Context object - * @deprecated This stuff should go in the renderers of a specific device. - */ - @Deprecated - public SharedContext(UserAgentCallback uac, FontResolver fr, ReplacedElementFactory ref, TextRenderer tr, float dpi) { - fontResolver = fr; - replacedElementFactory = ref; - setMedia("screen"); - this.uac = uac; - setCss(new StyleReference(uac)); - XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.RENDER_USING_CSS_IMPLEMENTATION_FROM, getCss().getClass().getName()); - setTextRenderer(tr); - setDPI(dpi); - } - - public LayoutContext newLayoutContextInstance() { LayoutContext c = new LayoutContext(this); return c; diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java index c14ff2e11..a6e4db1bc 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java @@ -255,11 +255,9 @@ private Element findFirstChild(Element parent, String targetName) { } protected StylesheetInfo readStyleElement(Element style) { - String media = style.getAttribute("media"); - if ("".equals(media)) { - media = "all"; - }//default for HTML is "screen", but that is silly and firefox seems to assume "all" StylesheetInfo info = new StylesheetInfo(); + String media = style.getAttribute("media"); + info.setMedia(media); info.setType(style.getAttribute("type")); info.setTitle(style.getAttribute("title")); @@ -306,12 +304,9 @@ protected StylesheetInfo readLinkElement(Element link) { info.setType(type); info.setOrigin(StylesheetInfo.AUTHOR); - info.setUri(link.getAttribute("href")); + String media = link.getAttribute("media"); - if ("".equals(media)) { - media = "all"; - } info.setMedia(media); String title = link.getAttribute("title"); diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java index 32985bfc4..67d43e529 100644 --- a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java +++ b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java @@ -195,6 +195,7 @@ public String formatMessage(Object[] args) { } enum LogMessageId2Param implements LogMessageId { + CSS_PARSE_TOO_MANY_STYLESHEET_IMPORTS(XRLog.CSS_PARSE, "Gave up after {} attempts to load stlyesheet at {} to avoid possible loop"), CSS_PARSE_COULDNT_PARSE_STYLESHEET_AT_URI(XRLog.CSS_PARSE, "Couldn't parse stylesheet at URI {}: {}"), CSS_PARSE_GENERIC_MESSAGE(XRLog.CSS_PARSE, "({}) {}"), diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/expected/css-import-loop.pdf b/openhtmltopdf-examples/src/main/resources/visualtest/expected/css-import-loop.pdf new file mode 100644 index 0000000000000000000000000000000000000000..179a36640f8fcb5053fe83a9b31fbefe6e2b7378 GIT binary patch literal 1092 zcmah|O>f#j5WSC-|1cL6qAKi%zd{v7im{Q>CMqRvb3i#bcmX54OKqd3{jA*U#TSKp;o+w1mq`V9B?&LN-Y2bYBTL{=#Rc2F8NdvzD{%T+Y+IqK2&+7$B z9@vJenj|SsfT}owhhIVBJK(qk!SN0zk3`Ji3phRs;bH;yYv-!@bNnrBWdmL@`9-hPT( z;zu~Kx4%7G>jSGvtHF?z!+?N&quBK>K;Xr}wq3W|TN~Oe!0tCsb>m743SePcsU|;5 z@=DL$0rNmAAm2EuKp?qf_^R?*{sw=Xf}y8+sXnF4GRwJ52cf zBa8>s@IjoEB*FpaILC2b{}}TfdWQ#G-zO)wSY4+1N|nts9UDW2ZvBJv~ F@CS7d4ORdE literal 0 HcmV?d00001 diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html b/openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html new file mode 100644 index 000000000..2f52844cf --- /dev/null +++ b/openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html @@ -0,0 +1,14 @@ + + + + + + +
RED
+
BLUE
+ + diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css b/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css new file mode 100644 index 000000000..ad2bba3c2 --- /dev/null +++ b/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css @@ -0,0 +1,3 @@ +@import "loop-two.css"; + +div#one { color: red; } diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css b/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css new file mode 100644 index 000000000..689b67b60 --- /dev/null +++ b/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css @@ -0,0 +1,3 @@ +@import "loop-one.css"; + +div#two { color: blue; } diff --git a/openhtmltopdf-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java b/openhtmltopdf-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java index 3e2ef6b31..2aa6f9b07 100644 --- a/openhtmltopdf-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java +++ b/openhtmltopdf-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java @@ -1343,6 +1343,11 @@ public void testMoreRobustDataUriBase64() throws IOException { assertTrue(vt.runTest("more-robust-data-uri-base64")); } + @Test + public void testCSSImportLoop() throws IOException { + assertTrue(vt.runTest("css-import-loop")); + } + // TODO: // + Elements that appear just on generated overflow pages. // + content property (page counters, etc) diff --git a/openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java b/openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java index ec7ef7654..5632b0b0c 100644 --- a/openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java +++ b/openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java @@ -233,11 +233,6 @@ private void setDocument(Document doc, String url, NamespaceHandler nsh) { //TODOgetFontResolver().flushFontFaceFonts(); - if (Configuration.isTrue("xr.cache.stylesheets", true)) { - _sharedContext.getCss().flushStyleSheets(); - } else { - _sharedContext.getCss().flushAllStyleSheets(); - } _sharedContext.setBaseURL(url); _sharedContext.setNamespaceHandler(nsh); _sharedContext.getCss().setDocumentContext(_sharedContext, _sharedContext.getNamespaceHandler(), doc, new NullUserInterface()); diff --git a/openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java b/openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java index 6813030ab..a533d7aeb 100644 --- a/openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java +++ b/openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java @@ -314,11 +314,6 @@ private void setDocumentP(Document doc, String url, NamespaceHandler nsh) { getFontResolver().flushFontFaceFonts(); - if (Configuration.isTrue("xr.cache.stylesheets", true)) { - _sharedContext.getCss().flushStyleSheets(); - } else { - _sharedContext.getCss().flushAllStyleSheets(); - } _sharedContext.setBaseURL(url); _sharedContext.setNamespaceHandler(nsh); _sharedContext.getCss().setDocumentContext(_sharedContext, _sharedContext.getNamespaceHandler(), doc, new NullUserInterface());