Skip to content
This repository was archived by the owner on Apr 10, 2025. It is now read-only.

Commit 95ef580

Browse files
jeffkaufmanjeffkaufman
authored
strip-subresource-hints: respect preserve with rel=preload, use href and not src (#1394)
* With rel=preload the hint tells us what type of resource it is, and if urls have been preserved for that type we should not strip it. * If the rel=preload type isn't image, script, or style we shouldn't strip it, because those are the only urls we change. * The filter was originally written to use src= when it should have used href=, which meant it removed hints it shouldn't have. This is a minimal change for backporting for branch 33. For the master branch I have a draft of a more complex change that does additional cleanups. Fixes: #1392 Fixes: #1393
1 parent 18d5549 commit 95ef580

File tree

11 files changed

+144
-65
lines changed

11 files changed

+144
-65
lines changed

‎install/mod_pagespeed_test/strip_subresource_hints/default/disallowtest.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<html>
2-
<head><link rel="subresource" src="dontrewriteme_resource.jpg"/></head>
2+
<head><link rel="subresource" href="dontrewriteme_resource.jpg"/></head>
33
<body>
44
</body>
55
</html>

‎install/mod_pagespeed_test/strip_subresource_hints/default/index.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<html>
2-
<head><link rel="subresource" src="test"/></head>
2+
<head><link rel="subresource" href="test"/></head>
33
<body>
44
</body>
55
</html>

‎install/mod_pagespeed_test/strip_subresource_hints/default/multiple_subresource_hints.html

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<html>
22
<head>
3-
<link rel="subresource" src="test1"/>
4-
<link rel="subresource" src="test2"/>
3+
<link rel="subresource" href="test1"/>
4+
<link rel="subresource" href="test2"/>
55
</head>
66
<body>
77
</body>

‎install/mod_pagespeed_test/strip_subresource_hints/default_passthrough/index.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<html>
2-
<head><link rel="subresource" src="test"/></head>
2+
<head><link rel="subresource" href="test"/></head>
33
<body>
44
</body>
55
</html>

‎install/mod_pagespeed_test/strip_subresource_hints/preserve_off/index.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<html>
2-
<head><link rel="subresource" src="test"/></head>
2+
<head><link rel="subresource" href="test"/></head>
33
<body>
44
</body>
55
</html>

‎install/mod_pagespeed_test/strip_subresource_hints/preserve_on/index.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<html>
2-
<head><link rel="subresource" src="test"/></head>
2+
<head><link rel="subresource" href="test"/></head>
33
<body>
44
</body>
55
</html>

‎net/instaweb/rewriter/public/strip_subresource_hints_filter.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ class StripSubresourceHintsFilter : public EmptyHtmlFilter {
4343
private:
4444
RewriteDriver* driver_;
4545
HtmlElement* delete_element_;
46-
bool remove_;
46+
bool remove_script_;
47+
bool remove_style_;
48+
bool remove_image_;
49+
bool remove_any_;
4750

4851
DISALLOW_COPY_AND_ASSIGN(StripSubresourceHintsFilter);
4952
};

‎net/instaweb/rewriter/strip_subresource_hints_filter.cc

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,32 +32,48 @@ namespace net_instaweb {
3232
StripSubresourceHintsFilter::StripSubresourceHintsFilter(RewriteDriver* driver)
3333
: driver_(driver),
3434
delete_element_(NULL),
35-
remove_(false) {
35+
remove_script_(false),
36+
remove_style_(false),
37+
remove_image_(false),
38+
remove_any_(false) {
3639
}
3740

3841
StripSubresourceHintsFilter::~StripSubresourceHintsFilter() { }
3942

4043
void StripSubresourceHintsFilter::StartDocument() {
4144
const RewriteOptions *options = driver_->options();
42-
remove_ = (driver_->can_modify_urls() &&
43-
(!(options->css_preserve_urls() &&
44-
options->image_preserve_urls() &&
45-
options->js_preserve_urls())));
45+
remove_script_ = driver_->can_modify_urls() && !options->js_preserve_urls();
46+
remove_style_ = driver_->can_modify_urls() && !options->css_preserve_urls();
47+
remove_image_ = driver_->can_modify_urls() && !options->image_preserve_urls();
48+
remove_any_ = remove_script_ || remove_style_ || remove_image_;
4649
delete_element_ = NULL;
4750
}
4851

4952
void StripSubresourceHintsFilter::StartElement(HtmlElement* element) {
50-
if (!remove_ || delete_element_) return;
53+
if (!remove_any_ || delete_element_) return;
5154

5255
if (element->keyword() == HtmlName::kLink) {
53-
const char *value = element->AttributeValue(HtmlName::kRel);
54-
if (value && (
55-
StringCaseEqual(value, "subresource") ||
56-
StringCaseEqual(value, "preload"))) {
56+
// Strip:
57+
// <link rel=subresource href=...> regardless
58+
// <link rel=preload as=script href=...> unless preserve scripts
59+
// <link rel=preload as=style href=...> unless preserve styles
60+
// <link rel=preload as=image href=...> unless preserve images
61+
//
62+
// Don't strip other kinds of rel=preload hints, because we don't change
63+
// their urls, so existing ones are still valid.
64+
const char* rel_value;
65+
const char* as_value;
66+
if ((rel_value = element->AttributeValue(HtmlName::kRel)) != NULL &&
67+
(StringCaseEqual(rel_value, "subresource") ||
68+
(StringCaseEqual(rel_value, "preload") &&
69+
(as_value = element->AttributeValue(HtmlName::kAs)) != NULL &&
70+
((remove_script_ && StringCaseEqual(as_value, "script")) ||
71+
(remove_style_ && StringCaseEqual(as_value, "style")) ||
72+
(remove_image_ && StringCaseEqual(as_value, "image")))))) {
5773
const RewriteOptions *options = driver_->options();
58-
const char *resource_url = element->AttributeValue(HtmlName::kSrc);
74+
const char *resource_url = element->AttributeValue(HtmlName::kHref);
5975
if (!resource_url) {
60-
// There's either no src attr, or one that we can't decode (utf8 etc).
76+
// There's either no href attr, or one that we can't decode (utf8 etc).
6177
// One way this could happen is if we have a url-encoded utf8 url in an
6278
// img tag and a utf8 encoded url in the subresource tag. Delete the
6379
// subresource link to be on the safe side.

‎net/instaweb/rewriter/strip_subresource_hints_filter_test.cc

Lines changed: 103 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,28 @@ class StripSubresourceHintsFilterTest :
8888
public StripSubresourceHintsFilterTestBase {
8989
};
9090

91-
class StripSubresourceHintsFilterTestPartialPreserve :
92-
public StripSubresourceHintsFilterTestBase {
91+
class StripSubresourceHintsFilterTestPreserveStyle :
92+
public StripSubresourceHintsFilterTestBase {
93+
protected:
94+
virtual void CustomSetup() {
95+
options()->set_css_preserve_urls(true);
96+
options()->set_js_preserve_urls(false);
97+
options()->set_image_preserve_urls(false);
98+
}
99+
};
100+
101+
class StripSubresourceHintsFilterTestPreserveScript :
102+
public StripSubresourceHintsFilterTestBase {
103+
protected:
104+
virtual void CustomSetup() {
105+
options()->set_css_preserve_urls(false);
106+
options()->set_js_preserve_urls(true);
107+
options()->set_image_preserve_urls(false);
108+
}
109+
};
110+
111+
class StripSubresourceHintsFilterTestPreserveImage :
112+
public StripSubresourceHintsFilterTestBase {
93113
protected:
94114
virtual void CustomSetup() {
95115
options()->set_css_preserve_urls(false);
@@ -148,7 +168,7 @@ TEST_F(StripSubresourceHintsFilterTest, SingleResourceNoLink) {
148168

149169
TEST_F(StripSubresourceHintsFilterTest, SingleResourceValidLink) {
150170
const char *source =
151-
"<head><link rel=\"subresource\" src=\"/test.gif\"/></head>"
171+
"<head><link rel=\"subresource\" href=\"/test.gif\"/></head>"
152172
"<body><img src=\"1.jpg\"/></body>";
153173
const char *rewritten =
154174
"<head></head>"
@@ -158,7 +178,7 @@ TEST_F(StripSubresourceHintsFilterTest, SingleResourceValidLink) {
158178

159179
TEST_F(StripSubresourceHintsFilterTest, SingleResourceValidPreloadLink) {
160180
const char *source =
161-
"<head><link rel=\"preload\" src=\"/test.gif\" as=\"image\"/></head>"
181+
"<head><link rel=\"preload\" href=\"/test.gif\" as=\"image\"/></head>"
162182
"<body><img src=\"1.jpg\"/></body>";
163183
const char *rewritten =
164184
"<head></head>"
@@ -169,7 +189,7 @@ TEST_F(StripSubresourceHintsFilterTest, SingleResourceValidPreloadLink) {
169189
TEST_F(StripSubresourceHintsFilterTest, SingleResourceExternalLink) {
170190
const char *source =
171191
"<head>"
172-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
192+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
173193
"</head>"
174194
"<body><img src=\"1.jpg\"/></body>";
175195
ValidateStripSubresourceHint(source, source);
@@ -178,21 +198,21 @@ TEST_F(StripSubresourceHintsFilterTest, SingleResourceExternalLink) {
178198
TEST_F(StripSubresourceHintsFilterTest, MultiResourceMixedLinks) {
179199
const char *source =
180200
"<head>"
181-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
182-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
201+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
202+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
183203
"</head>"
184204
"<body><img src=\"1.jpg\"/></body>";
185205
const char *rewritten =
186206
"<head>"
187-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
207+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
188208
"</head>"
189209
"<body><img src=\"1.jpg\"/></body>";
190210
ValidateStripSubresourceHint(source, rewritten);
191211
}
192212

193213
TEST_F(StripSubresourceHintsFilterTest, SingleResourceRewriteDomain) {
194214
const char *source =
195-
"<head><link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
215+
"<head><link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
196216
"</head>"
197217
"<body><img src=\"1.jpg\"/></body>";
198218
const char *rewritten =
@@ -203,30 +223,32 @@ TEST_F(StripSubresourceHintsFilterTest, SingleResourceRewriteDomain) {
203223

204224
TEST_F(StripSubresourceHintsFilterTest, SingleResourceDisallow) {
205225
const char *source =
206-
"<head><link rel=\"subresource\" src=\"/dontdropme/test.gif\"/>"
226+
"<head><link rel=\"subresource\" href=\"/dontdropme/test.gif\"/>"
207227
"</head>"
208228
"<body><img src=\"1.jpg\"/></body>";
209229
const char *rewritten =
210-
"<head><link rel=\"subresource\" src=\"/dontdropme/test.gif\"/>"
230+
"<head><link rel=\"subresource\" href=\"/dontdropme/test.gif\"/>"
211231
"</head>"
212232
"<body><img src=\"1.jpg\"/></body>";
213233
ValidateStripSubresourceHint(source, rewritten);
214234
}
215235

216-
TEST_F(StripSubresourceHintsFilterTestPartialPreserve,
217-
MultiResourcePreserveAll) {
236+
// Even if you turn on preserve images, we still strip all rel=subresource hints
237+
// because we don't know which ones are images.
238+
TEST_F(StripSubresourceHintsFilterTestPreserveImage,
239+
MultiSubresourcePreserveImages) {
218240
const char *source =
219241
"<head>"
220-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
221-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
222-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
223-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
242+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
243+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
244+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
245+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
224246
"</head>"
225247
"<body><img src=\"1.jpg\"/></body>";
226248
const char *rewritten =
227249
"<head>"
228-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
229-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
250+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
251+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
230252
"</head>"
231253
"<body><img src=\"1.jpg\"/></body>";
232254
ValidateStripSubresourceHint(source, rewritten);
@@ -238,23 +260,59 @@ TEST_F(StripSubresourceHintsFilterTestFullPreserve,
238260
MultiResourcePreserveAll) {
239261
const char *source =
240262
"<head>"
241-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
242-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
243-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
244-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
263+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
264+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
265+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
266+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
245267
"</head>"
246268
"<body><img src=\"1.jpg\"/></body>";
247269
const char *rewritten =
248270
"<head>"
249-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
250-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
251-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
252-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
271+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
272+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
273+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
274+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
253275
"</head>"
254276
"<body><img src=\"1.jpg\"/></body>";
255277
ValidateStripSubresourceHint(source, rewritten);
256278
}
257279

280+
// With rel=preload, if you have set preserve for a type we don't strip it.
281+
TEST_F(StripSubresourceHintsFilterTestPreserveImage, ImagesPreserved) {
282+
const char* source =
283+
"<link rel=preload as=image href=a.jpg>"
284+
"<link rel=preload as=script href=a.js>"
285+
"<link rel=preload as=style href=a.css>";
286+
const char* rewritten =
287+
"<link rel=preload as=image href=a.jpg>";
288+
ValidateStripSubresourceHint(source, rewritten);
289+
}
290+
TEST_F(StripSubresourceHintsFilterTestPreserveScript, ScriptsPreserved) {
291+
const char* source =
292+
"<link rel=preload as=image href=a.jpg>"
293+
"<link rel=preload as=script href=a.js>"
294+
"<link rel=preload as=style href=a.css>";
295+
const char* rewritten =
296+
"<link rel=preload as=script href=a.js>";
297+
ValidateStripSubresourceHint(source, rewritten);
298+
}
299+
TEST_F(StripSubresourceHintsFilterTestPreserveStyle, StylesPreserved) {
300+
const char* source =
301+
"<link rel=preload as=image href=a.jpg>"
302+
"<link rel=preload as=script href=a.js>"
303+
"<link rel=preload as=style href=a.css>";
304+
const char* rewritten =
305+
"<link rel=preload as=style href=a.css>";
306+
ValidateStripSubresourceHint(source, rewritten);
307+
}
308+
309+
// With rel=preload we don't strip unknown types.
310+
TEST_F(StripSubresourceHintsFilterTest, DontStripUnknownTypes) {
311+
const char* source = "<link rel=preload as=font href=a.woff>";
312+
ValidateStripSubresourceHint(source, source);
313+
}
314+
315+
258316
TEST_F(StripSubresourceHintsFilterTestDisabled,
259317
PreserveSubResourceHintsIsTrue) {
260318
can_modify_urls_filter_->set_can_modify_urls(true);
@@ -266,10 +324,10 @@ TEST_F(StripSubresourceHintsFilterTestDisabled,
266324
can_modify_urls_filter_->set_can_modify_urls(true);
267325
const char *source =
268326
"<head>"
269-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
270-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
271-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
272-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
327+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
328+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
329+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
330+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
273331
"</head>"
274332
"<body><img src=\"1.jpg\"/></body>";
275333
ValidateStripSubresourceHint(source, source);
@@ -279,18 +337,18 @@ TEST_F(StripSubresourceHintsFilterTestRewriteLevelPassthrough,
279337
MultiResource) {
280338
const char *source =
281339
"<head>"
282-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
283-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
284-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
285-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
340+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
341+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
342+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
343+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
286344
"</head>"
287345
"<body></body>";
288346
const char *rewritten =
289347
"<head>"
290-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
291-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
292-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
293-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
348+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
349+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
350+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
351+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
294352
"</head>"
295353
"<body></body>";
296354
ValidateExpected("multi_resource", source, rewritten);
@@ -300,16 +358,16 @@ TEST_F(StripSubresourceHintsFilterTestRewriteLevelCoreFilters,
300358
MultiResource) {
301359
const char *source =
302360
"<head>"
303-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
304-
"<link rel=\"subresource\" src=\"/test.gif\"/>"
305-
"<link rel=\"subresource\" src=\"http://from1.test.com/test.gif\"/>"
306-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
361+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
362+
"<link rel=\"subresource\" href=\"/test.gif\"/>"
363+
"<link rel=\"subresource\" href=\"http://from1.test.com/test.gif\"/>"
364+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
307365
"</head>"
308366
"<body></body>";
309367
const char *rewritten =
310368
"<head>"
311-
"<link rel=\"subresource\" src=\"/dontdropme.gif\"/>"
312-
"<link rel=\"subresource\" src=\"http://www.example.com/test.gif\"/>"
369+
"<link rel=\"subresource\" href=\"/dontdropme.gif\"/>"
370+
"<link rel=\"subresource\" href=\"http://www.example.com/test.gif\"/>"
313371
"</head>"
314372
"<body></body>";
315373
ValidateExpected("multi_resource", source, rewritten);

‎pagespeed/kernel/html/html_name.gperf

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ struct KeywordMap {const char* name; net_instaweb::HtmlName::Keyword keyword;};
3333
"alt", HtmlName::kAlt
3434
"area", HtmlName::kArea
3535
"article", HtmlName::kArticle
36+
"as", HtmlName::kAs
3637
"aside", HtmlName::kAside
3738
"async", HtmlName::kAsync
3839
"audio", HtmlName::kAudio

0 commit comments

Comments
 (0)