Permalink
Browse files

Stop appending ?PageSpeed=off to .pagespeed. resources in the sources…

… list for source_maps.

We must append ?PageSpeed=off to non-pagespeed URLs because otherwise IPRO might rewrite the source and lead to a non-sensical source map. However, IPRO does not modify .pages
peed. URLs, so we can safely avoid this query param. Furthermore, the param does not make sense for .pagespeed. resources and seems to 404.

Fixes issue 1043.
backported from Stop appending ?PageSpeed=off to .pagespeed. resources in the sources list for source_maps.

We must append ?PageSpeed=off to non-pagespeed URLs because otherwise IPRO might rewrite the source and lead to a non-sensical source map. However, IPRO does not modify .pages
peed. URLs, so we can safely avoid this query param. Furthermore, the param does not make sense for .pagespeed. resources and seems to 404.

Fixes issue 1043.
backported from 701c6c0
  • Loading branch information...
crowell committed May 11, 2015
1 parent 0f2a6fc commit 9d6d08782d4289b6d71fee3bf5406cf65e3e0c0b
Showing with 64 additions and 5 deletions.
  1. +11 −5 net/instaweb/rewriter/javascript_filter.cc
  2. +53 −0 net/instaweb/rewriter/javascript_filter_test.cc
@@ -168,12 +168,18 @@ class JavascriptFilter::Context : public SingleRewriteContext {
if (Options()->Enabled(RewriteOptions::kIncludeJsSourceMaps) &&
// Source map will be empty if we can't construct it correctly.
!code_block.SourceMappings().empty()) {
// Note: We append PageSpeed=off query parameter to make sure that
// the source URL doesn't get rewritten with IPRO.
GoogleUrl original_gurl(input->url());
scoped_ptr<GoogleUrl> source_gurl(
original_gurl.CopyAndAddEscapedQueryParam(RewriteQuery::kPageSpeed,
"off"));
scoped_ptr<GoogleUrl> source_gurl;
if (server_context->IsPagespeedResource(original_gurl)) {
// Do not append Pagespeed=off if input is already a pagespeed resource.
source_gurl.reset(new GoogleUrl);
source_gurl->Reset(original_gurl);
} else {
// Note: We append PageSpeed=off query parameter to make sure that
// the source URL doesn't get rewritten with IPRO.
source_gurl.reset(original_gurl.CopyAndAddEscapedQueryParam(
RewriteQuery::kPageSpeed, "off"));
}

GoogleString source_map_text;
// Note: We omit rewritten URL because of a chicken-and-egg problem.
@@ -32,6 +32,7 @@
#include "net/instaweb/rewriter/public/debug_filter.h"
#include "net/instaweb/rewriter/public/javascript_code_block.h"
#include "net/instaweb/rewriter/public/javascript_library_identification.h"
#include "net/instaweb/rewriter/public/js_outline_filter.h"
#include "net/instaweb/rewriter/public/rewrite_driver.h"
#include "net/instaweb/rewriter/public/rewrite_options.h"
#include "net/instaweb/rewriter/public/rewrite_test_base.h"
@@ -1241,6 +1242,58 @@ TEST_P(JavascriptFilterTest, SourceMapUnsanitaryUrl) {
EXPECT_EQ(expected_output_js, output_js);
}

// For non-pagespeed input JS, we must add ?PageSpeed=off to the source URL
// to avoid IPRO rewriting the source. However, we should not add ?PageSpeed=off
// for .pagespeed. input files, because that doesn't make any sense.
// https://code.google.com/p/modpagespeed/issues/detail?id=1043
TEST_P(JavascriptFilterTest, ProperSourceMapForPagespeedInput) {
if (!options()->use_experimental_js_minifier()) return;

options()->EnableFilter(RewriteOptions::kIncludeJsSourceMaps);
options()->EnableFilter(RewriteOptions::kRewriteJavascriptExternal);
options()->EnableFilter(RewriteOptions::kOutlineJavascript);
options()->set_js_outline_min_bytes(0); // Make sure everything is outlined.
UseMd5Hasher(); // We should use a real hasher for outlining.
rewrite_driver_->AddFilters();

const char input_js[] = "foo = 1;";
GoogleString outlined_js_tail =
Encode("", JsOutlineFilter::kFilterId,
hasher()->Hash(input_js), "_", "js");

const char expected_mapping_vlq[] = "AAAA,GAAI,CAAE";
// Note: No ?PageSpeed=off
GoogleString expected_source_map = StrCat(
")]}'\n{\"mappings\":\"", expected_mapping_vlq, "\",\"names\":[],"
"\"sources\":[\"", kTestDomain, outlined_js_tail, "\"],"
"\"version\":3}\n");
GoogleString source_map_url =
Encode(kTestDomain, RewriteOptions::kJavascriptMinSourceMapId,
hasher()->Hash(expected_source_map), outlined_js_tail, "map");

const char rewritten_js[] = "foo=1;";
GoogleString expected_output_js =
StrCat(rewritten_js, "\n"
"//# sourceMappingURL=", source_map_url, "\n");
GoogleString rewritten_js_url =
Encode(kTestDomain, RewriteOptions::kJavascriptMinId,
hasher()->Hash(expected_output_js), outlined_js_tail, "js");

GoogleString input_html = StrCat("<script>", input_js, "</script>");
GoogleString expected_output_html =
StrCat("<script src=\"", rewritten_js_url, "\"></script>");
ValidateExpected("source_map_pagespeed", input_html, expected_output_html);

GoogleString output_js;
EXPECT_TRUE(FetchResourceUrl(rewritten_js_url, &output_js));
EXPECT_EQ(expected_output_js, output_js);

GoogleString source_map;
EXPECT_TRUE(FetchResourceUrl(source_map_url, &source_map));
EXPECT_EQ(expected_source_map, source_map);
}


TEST_P(JavascriptFilterTest, InlineAndNotExternal) {
options()->EnableFilter(RewriteOptions::kRewriteJavascriptInline);
options()->DisableFilter(RewriteOptions::kRewriteJavascriptExternal);

0 comments on commit 9d6d087

Please sign in to comment.