Skip to content

Commit 34aaf6a

Browse files
danfickledanfickle
committed
#467 (related) Prevent CSS import loop
+ With test. + Cleanup of CSS code. + Removed stylesheet cache.
1 parent 2d8edb7 commit 34aaf6a

File tree

14 files changed

+74
-146
lines changed

14 files changed

+74
-146
lines changed

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
import com.openhtmltopdf.css.extend.lib.DOMTreeResolver;
3737
import com.openhtmltopdf.css.newmatch.CascadedStyle;
3838
import com.openhtmltopdf.css.newmatch.PageInfo;
39-
import com.openhtmltopdf.css.newmatch.Selector;
4039
import com.openhtmltopdf.css.parser.CSSPrimitiveValue;
4140
import com.openhtmltopdf.css.sheet.PropertyDeclaration;
4241
import com.openhtmltopdf.css.sheet.Stylesheet;
@@ -211,29 +210,6 @@ public PageInfo getPageStyle(String pageName, String pseudoPage) {
211210
return _matcher.getPageCascadedStyle(pageName, pseudoPage);
212211
}
213212

214-
/**
215-
* Flushes any stylesheet associated with this stylereference (based on the user agent callback) that are in cache.
216-
* Deprecated for now, until we fix caching, use a new <code>StylesheetFactory</code> each run.
217-
*/
218-
@Deprecated
219-
public void flushStyleSheets() {
220-
String uri = _uac.getBaseURL();
221-
StylesheetInfo info = new StylesheetInfo();
222-
info.setUri(uri);
223-
info.setOrigin(StylesheetInfo.AUTHOR);
224-
if (_stylesheetFactory.containsStylesheet(uri)) {
225-
_stylesheetFactory.removeCachedStylesheet(uri);
226-
XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.CSS_PARSE_REMOVING_STYLESHEET_URI_FROM_CACHE_BY_REQUEST, uri);
227-
} else {
228-
XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.CSS_PARSE_REQUESTED_REMOVING_STYLESHEET_URI_NOT_IN_CACHE, uri);
229-
}
230-
}
231-
232-
@Deprecated
233-
public void flushAllStyleSheets() {
234-
_stylesheetFactory.flushCachedStylesheets();
235-
}
236-
237213
/**
238214
* Gets StylesheetInfos for all stylesheets and inline styles associated
239215
* with the current document. Default (user agent) stylesheet and the inline

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java

Lines changed: 23 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import java.io.IOException;
2323
import java.io.Reader;
24+
import java.util.HashMap;
25+
import java.util.Map;
2426
import java.util.logging.Level;
2527

2628
import com.openhtmltopdf.css.extend.StylesheetFactory;
@@ -46,21 +48,19 @@ public class StylesheetFactoryImpl implements StylesheetFactory {
4648
*/
4749
private UserAgentCallback _userAgentCallback;
4850

49-
private final int _cacheCapacity = 16;
51+
/**
52+
* This may avoid @import loops, ie. one.css includes two.css
53+
* which then includes one.css.
54+
*/
55+
private final Map<String, Integer> _seenStylesheetUris = new HashMap<>();
5056

5157
/**
52-
* an LRU cache
58+
* The maximum number of times a stylesheet uri can be link or
59+
* imported before we give up and conclude there is a loop.
5360
*/
54-
private final java.util.LinkedHashMap<String, Stylesheet> _cache =
55-
new java.util.LinkedHashMap<String, Stylesheet>(_cacheCapacity, 0.75f, true) {
56-
private static final long serialVersionUID = 1L;
61+
private static final int MAX_STYLESHEET_INCLUDES = 10;
5762

58-
protected boolean removeEldestEntry(java.util.Map.Entry<String, Stylesheet> eldest) {
59-
return size() > _cacheCapacity;
60-
}
61-
};
62-
63-
private CSSParser _cssParser;
63+
private final CSSParser _cssParser;
6464

6565
public StylesheetFactoryImpl(UserAgentCallback userAgentCallback) {
6666
_userAgentCallback = userAgentCallback;
@@ -108,74 +108,27 @@ public Ruleset parseStyleDeclaration(int origin, String styleDeclaration) {
108108
}
109109

110110
/**
111-
* Adds a stylesheet to the factory cache. Will overwrite older entry for
112-
* same key.
113-
*
114-
* @param key Key to use to reference sheet later; must be unique in
115-
* factory.
116-
* @param sheet The sheet to cache.
117-
*/
118-
@Deprecated
119-
public void putStylesheet(String key, Stylesheet sheet) {
120-
_cache.put(key, sheet);
121-
}
122-
123-
/**
124-
* @param key
125-
* @return true if a Stylesheet with this key has been put in the cache.
126-
* Note that the Stylesheet may be null.
127-
*/
128-
//TODO: work out how to handle caching properly, with cache invalidation
129-
@Deprecated
130-
public boolean containsStylesheet(String key) {
131-
return _cache.containsKey(key);
132-
}
133-
134-
/**
135-
* Returns a cached sheet by its key; null if no entry for that key.
136-
*
137-
* @param key The key for this sheet; same as key passed to
138-
* putStylesheet();
139-
* @return The stylesheet
140-
*/
141-
@Deprecated
142-
public Stylesheet getCachedStylesheet(String key) {
143-
return _cache.get(key);
144-
}
145-
146-
/**
147-
* Removes a cached sheet by its key.
148-
*
149-
* @param key The key for this sheet; same as key passed to
150-
* putStylesheet();
151-
*/
152-
@Deprecated
153-
public Object removeCachedStylesheet(String key) {
154-
return _cache.remove(key);
155-
}
156-
157-
@Deprecated
158-
public void flushCachedStylesheets() {
159-
_cache.clear();
160-
}
161-
162-
/**
163-
* Returns a cached sheet by its key; loads and caches it if not in cache;
111+
* Returns a sheet by its key
164112
* null if not able to load
165113
*
114+
*
166115
* @param info The StylesheetInfo for this sheet
167116
* @return The stylesheet
168117
*/
169-
//TODO: this looks a bit odd
170118
public Stylesheet getStylesheet(StylesheetInfo info) {
171119
XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.LOAD_REQUESTING_STYLESHEET_AT_URI, info.getUri());
172120

173-
Stylesheet s = getCachedStylesheet(info.getUri());
174-
if (s == null && !containsStylesheet(info.getUri())) {
175-
s = parse(info);
176-
putStylesheet(info.getUri(), s);
121+
Integer includeCount = _seenStylesheetUris.get(info.getUri());
122+
123+
if (includeCount != null && includeCount >= MAX_STYLESHEET_INCLUDES) {
124+
// Probably an import loop.
125+
XRLog.log(Level.SEVERE, LogMessageId.LogMessageId2Param.CSS_PARSE_TOO_MANY_STYLESHEET_IMPORTS, includeCount, info.getUri());
126+
return null;
177127
}
178-
return s;
128+
129+
_seenStylesheetUris.merge(info.getUri(), 1, (oldV, newV) -> oldV + 1);
130+
131+
return parse(info);
179132
}
180133

181134
public void setUserAgentCallback(UserAgentCallback userAgent) {

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ private void import_rule(Stylesheet stylesheet) throws IOException {
300300
t, new Token[] { Token.TK_STRING, Token.TK_URI }, getCurrentLine());
301301
}
302302

303-
if (info.getMedia().size() == 0) {
303+
if (info.getMedia().isEmpty()) {
304304
info.addMedium("all");
305305
}
306306
stylesheet.addImportRule(info);

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package com.openhtmltopdf.css.sheet;
2121

2222
import java.util.ArrayList;
23+
import java.util.Arrays;
2324
import java.util.List;
2425
import java.util.Locale;
2526

@@ -38,12 +39,12 @@
3839
*/
3940
public class StylesheetInfo {
4041

41-
private Stylesheet stylesheet = null;//just to be able to attach "dummy" stylesheets. Also might save a lookup if it's already looked up
42+
private Stylesheet stylesheet = null;
4243
private String title;
4344
private String uri;
4445
private int origin = USER_AGENT;
4546
private String type;
46-
private List<String> mediaTypes = new ArrayList<>();
47+
private final List<String> mediaTypes = new ArrayList<>();
4748
private String content;
4849

4950
/** Origin of stylesheet - user agent */
@@ -54,15 +55,20 @@ public class StylesheetInfo {
5455

5556
/** Origin of stylesheet - author */
5657
public final static int AUTHOR = 2;
57-
5858

5959
/**
6060
* @param m a single media identifier
6161
* @return true if the stylesheet referenced applies to the medium
6262
*/
6363
public boolean appliesToMedia(String m) {
64-
return m.toLowerCase(Locale.US).equals("all") ||
65-
mediaTypes.contains("all") || mediaTypes.contains(m.toLowerCase(Locale.US));
64+
if (mediaTypes.contains("all")) {
65+
return true;
66+
}
67+
68+
String media = m.toLowerCase(Locale.US);
69+
70+
return media.equals("all") ||
71+
mediaTypes.contains(media);
6672
}
6773

6874
/**
@@ -80,18 +86,18 @@ public void setUri(String uri) {
8086
* @param media The new media value
8187
*/
8288
public void setMedia(String media) {
83-
String[] mediaTypes = media.split(",");
8489
this.mediaTypes.clear();
85-
86-
for (int i = 0; i < mediaTypes.length; i++) {
87-
this.mediaTypes.add(mediaTypes[i].trim().toLowerCase(Locale.US));
90+
91+
if (media == null || media.isEmpty()) {
92+
// Common case, no media attribute, applies to all.
93+
this.addMedium("all");
94+
} else {
95+
Arrays.stream(media.split(","))
96+
.map(mediaType -> mediaType.trim().toLowerCase(Locale.US))
97+
.forEach(this::addMedium);
8898
}
8999
}
90-
91-
public void setMedia(List<String> mediaTypes) {
92-
this.mediaTypes = mediaTypes;
93-
}
94-
100+
95101
public void addMedium(String medium) {
96102
mediaTypes.add(medium);
97103
}
@@ -193,7 +199,7 @@ public String getContent() {
193199
public void setContent(String content) {
194200
this.content = content;
195201
}
196-
202+
197203
public boolean isInline() {
198204
return this.content != null;
199205
}

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -129,24 +129,6 @@ public class SharedContext {
129129
public SharedContext() {
130130
}
131131

132-
133-
/**
134-
* Constructor for the Context object
135-
* @deprecated This stuff should go in the renderers of a specific device.
136-
*/
137-
@Deprecated
138-
public SharedContext(UserAgentCallback uac, FontResolver fr, ReplacedElementFactory ref, TextRenderer tr, float dpi) {
139-
fontResolver = fr;
140-
replacedElementFactory = ref;
141-
setMedia("screen");
142-
this.uac = uac;
143-
setCss(new StyleReference(uac));
144-
XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.RENDER_USING_CSS_IMPLEMENTATION_FROM, getCss().getClass().getName());
145-
setTextRenderer(tr);
146-
setDPI(dpi);
147-
}
148-
149-
150132
public LayoutContext newLayoutContextInstance() {
151133
LayoutContext c = new LayoutContext(this);
152134
return c;

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -255,11 +255,9 @@ private Element findFirstChild(Element parent, String targetName) {
255255
}
256256

257257
protected StylesheetInfo readStyleElement(Element style) {
258-
String media = style.getAttribute("media");
259-
if ("".equals(media)) {
260-
media = "all";
261-
}//default for HTML is "screen", but that is silly and firefox seems to assume "all"
262258
StylesheetInfo info = new StylesheetInfo();
259+
String media = style.getAttribute("media");
260+
263261
info.setMedia(media);
264262
info.setType(style.getAttribute("type"));
265263
info.setTitle(style.getAttribute("title"));
@@ -306,12 +304,9 @@ protected StylesheetInfo readLinkElement(Element link) {
306304
info.setType(type);
307305

308306
info.setOrigin(StylesheetInfo.AUTHOR);
309-
310307
info.setUri(link.getAttribute("href"));
308+
311309
String media = link.getAttribute("media");
312-
if ("".equals(media)) {
313-
media = "all";
314-
}
315310
info.setMedia(media);
316311

317312
String title = link.getAttribute("title");

‎openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,7 @@ public String formatMessage(Object[] args) {
195195
}
196196

197197
enum LogMessageId2Param implements LogMessageId {
198+
CSS_PARSE_TOO_MANY_STYLESHEET_IMPORTS(XRLog.CSS_PARSE, "Gave up after {} attempts to load stlyesheet at {} to avoid possible loop"),
198199
CSS_PARSE_COULDNT_PARSE_STYLESHEET_AT_URI(XRLog.CSS_PARSE, "Couldn't parse stylesheet at URI {}: {}"),
199200
CSS_PARSE_GENERIC_MESSAGE(XRLog.CSS_PARSE, "({}) {}"),
200201

‎openhtmltopdf-examples/src/main/resources/visualtest/expected/css-import-loop.pdf

1.07 KB
Binary file not shown.

‎openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<html>
2+
<head>
3+
<style>
4+
@page {
5+
size: 200px 200px;
6+
}
7+
</style>
8+
<link rel="stylesheet" href="stylesheets/loop-one.css" />
9+
</head>
10+
<body>
11+
<div id="one">RED</div>
12+
<div id="two">BLUE</div>
13+
</body>
14+
</html>

‎openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
@import "loop-two.css";
2+
3+
div#one { color: red; }

‎openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
@import "loop-one.css";
2+
3+
div#two { color: blue; }

‎openhtmltopdf-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,6 +1343,11 @@ public void testMoreRobustDataUriBase64() throws IOException {
13431343
assertTrue(vt.runTest("more-robust-data-uri-base64"));
13441344
}
13451345

1346+
@Test
1347+
public void testCSSImportLoop() throws IOException {
1348+
assertTrue(vt.runTest("css-import-loop"));
1349+
}
1350+
13461351
// TODO:
13471352
// + Elements that appear just on generated overflow pages.
13481353
// + content property (page counters, etc)

‎openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -233,11 +233,6 @@ private void setDocument(Document doc, String url, NamespaceHandler nsh) {
233233

234234
//TODOgetFontResolver().flushFontFaceFonts();
235235

236-
if (Configuration.isTrue("xr.cache.stylesheets", true)) {
237-
_sharedContext.getCss().flushStyleSheets();
238-
} else {
239-
_sharedContext.getCss().flushAllStyleSheets();
240-
}
241236
_sharedContext.setBaseURL(url);
242237
_sharedContext.setNamespaceHandler(nsh);
243238
_sharedContext.getCss().setDocumentContext(_sharedContext, _sharedContext.getNamespaceHandler(), doc, new NullUserInterface());

‎openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -314,11 +314,6 @@ private void setDocumentP(Document doc, String url, NamespaceHandler nsh) {
314314

315315
getFontResolver().flushFontFaceFonts();
316316

317-
if (Configuration.isTrue("xr.cache.stylesheets", true)) {
318-
_sharedContext.getCss().flushStyleSheets();
319-
} else {
320-
_sharedContext.getCss().flushAllStyleSheets();
321-
}
322317
_sharedContext.setBaseURL(url);
323318
_sharedContext.setNamespaceHandler(nsh);
324319
_sharedContext.getCss().setDocumentContext(_sharedContext, _sharedContext.getNamespaceHandler(), doc, new NullUserInterface());

0 commit comments

Comments
 (0)