Always parse process config into RewriteDriverFactory::default_options,

and then sync it back to per-ServerContext options. Should fix the regression with ImageMaxRewritesAtOnce (#1305) and give RewriteDriverFactory and all the ServerContexts a consistent view of their value.
apache · May 19, 2016 · 19cf3bb60b2aadff90ec588aab780e72daf0b581 · 19cf3bb
1 parent 6dfe527
commit 19cf3bb60b2aadff90ec588aab780e72daf0b581
diff --git a/install/debug.conf.template b/install/debug.conf.template
@@ -1761,6 +1761,24 @@ ModPagespeedMessagesDomains Allow localhost
  ModPagespeed off
 </VirtualHost>
+  
+# For testing how we handle process-scope options.
+  
+ModPagespeedIproMaxResponseBytes 1048576001
+  
+<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
+  
+  ServerName ps1.example.com
+  
+  DocumentRoot "@@APACHE_DOC_ROOT@@"
+  
+  ModPagespeedFileCachePath "@@MOD_PAGESPEED_CACHE@@_mpr"
+  
+  ModPagespeedIproMaxResponseBytes 1048576002
+  
+  ModPagespeedEnableFilters debug
+  
+</VirtualHost>
+  
+
+  
+<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
+  
+  ServerName ps2.example.com
+  
+  DocumentRoot "@@APACHE_DOC_ROOT@@"
+  
+  ModPagespeedFileCachePath "@@MOD_PAGESPEED_CACHE@@_mpr"
+  
+  ModPagespeedIproMaxResponseBytes 1048576003
+  
+  ModPagespeedEnableFilters debug
+  
+</VirtualHost>
+  
+
 # For testing with a custom origin header.  In this VirtualHost,
 # /mod_pagespeed_test is included in our DocumentRoot and thus does
 # not need to be in any resource URL paths.  This helps us verify that
diff --git a/net/instaweb/rewriter/public/rewrite_options.h b/net/instaweb/rewriter/public/rewrite_options.h
@@ -2853,6 +2853,10 @@ class RewriteOptions {
  // not true, this function will DCHECK.
  virtual void Merge(const RewriteOptions& src);
+  
+  // Merge the process scope options (including strict ones) from src into
+  
+  // this.
+  
+  void MergeOnlyProcessScopeOptions(const RewriteOptions& src);
+  
+
  // Registers a wildcard pattern for to be allowed, potentially overriding
  // previous Disallow wildcards.
  void Allow(StringPiece wildcard_pattern) {
diff --git a/net/instaweb/rewriter/rewrite_options.cc b/net/instaweb/rewriter/rewrite_options.cc
@@ -3937,6 +3937,28 @@ void RewriteOptions::Merge(const RewriteOptions& src) {
  }
 }
+  
+void RewriteOptions::MergeOnlyProcessScopeOptions(const RewriteOptions& src) {
+  
+  DCHECK(!frozen_);
+  
+#ifndef NDEBUG  // MergeOK is only around in CHECK-enabled builds.
+  
+  CHECK(src.MergeOK());
+  
+#endif
+  
+
+  
+  DCHECK_EQ(all_options_.size(), src.all_options_.size());
+  
+  DCHECK_EQ(initialized_options_, src.initialized_options_);
+  
+  DCHECK_EQ(initialized_options_, all_options_.size());
+  
+
+  
+  size_t options_to_merge = std::min(all_options_.size(),
+  
+                                     src.all_options_.size());
+  
+  for (size_t i = 0; i < options_to_merge; ++i) {
+  
+    OptionScope scope = all_options_[i]->scope();
+  
+    if (scope == kProcessScope || scope == kProcessScopeStrict) {
+  
+      all_options_[i]->Merge(src.all_options_[i]);
+  
+    }
+  
+  }
+  
+
+  
+  Modify();
+  
+}
+  
+
 RewriteOptions* RewriteOptions::Clone() const {
  RewriteOptions* options = NewOptions();
  options->Merge(*this);
diff --git a/net/instaweb/rewriter/rewrite_options_test.cc b/net/instaweb/rewriter/rewrite_options_test.cc
@@ -661,6 +661,20 @@ TEST_F(RewriteOptionsTest, MergeDistributed) {
  EXPECT_FALSE(options_.Distributable(RewriteOptions::kCssFilterId));
 }
+  
+TEST_F(RewriteOptionsTest, MergeOnlyProcessScopeOptions) {
+  
+  RewriteOptions dest(&thread_system_), src(&thread_system_);
+  
+  dest.set_image_max_rewrites_at_once(2);
+  
+  dest.set_max_url_segment_size(1);
+  
+  src.set_image_max_rewrites_at_once(5);
+  
+  src.set_max_url_segment_size(4);
+  
+
+  
+  dest.MergeOnlyProcessScopeOptions(src);
+  
+  // Pulled in set_image_max_rewrites_at_once, which is process scope,
+  
+  // but not the other option.
+  
+  EXPECT_EQ(5, dest.image_max_rewrites_at_once());
+  
+  EXPECT_EQ(1, dest.max_url_segment_size());
+  
+}
+  
+
 TEST_F(RewriteOptionsTest, Allow) {
  options_.Allow("*.css");
  EXPECT_TRUE(options_.IsAllowed("abcd.css"));
diff --git a/pagespeed/apache/mod_instaweb.cc b/pagespeed/apache/mod_instaweb.cc
@@ -332,9 +332,9 @@ class ApacheProcessContext {
    return factory_.get();
  }
-  
-  // Checks cmd to see if it's used in a vhost or conditional context, and
-  
-  // if so, if that's an error or warning condition.
-  
-  const char* CheckCommandForVhost(const cmd_parms* cmd);
+  
+  // Checks cmd to see if it's process scope, and if so if it's used in an
+  
+  // incorrect context, returning an error message if so.
+  
+  const char* CheckProcessScope(const cmd_parms* cmd, bool* is_process_scope);
  scoped_ptr<ApacheRewriteDriverFactory> factory_;
  // Process-scoped static variable cleanups, mainly for valgrind.
@@ -949,6 +949,18 @@ int pagespeed_post_config(apr_pool_t* pool, apr_pool_t* plog, apr_pool_t* ptemp,
      CHECK(server_context != NULL);
      server_contexts.push_back(server_context);
    }
+  
+
+  
+    // We also want propagate all the per-process options to each vhost. The
+  
+    // normal merge in merge_server_config isn't enough since that merges the
+  
+    // non-per process things from a dummy ServerContext corresponding to the
+  
+    // top-level config, not ApacheRewriteDriverFactory::default_options where
+  
+    // the process scope options go.
+  
+    //
+  
+    // We do this here rather than merge_server_config since we want to touch
+  
+    // the ServerContext corresponding to the top-level/non-<VirtualHost>
+  
+    // block, too.
+  
+    server_context->global_config()->MergeOnlyProcessScopeOptions(
+  
+        *factory->default_options());
  }
  GoogleString error_message;
@@ -1291,17 +1303,14 @@ static char* CheckGlobalOption(const cmd_parms* cmd,
  return NULL;
 }
-  
-const char* ApacheProcessContext::CheckCommandForVhost(const cmd_parms* cmd) {
-  
-  // Only do the vhost_command_handling_map_ lookup if it's going
-  
-  // to be used by CheckGlobalOption.
-  
-  //
-  
-  // TODO(jmarantz): Add a scope argument ParseAndSetOptionFromName[123] and
-  
-  // let it do the error-checking & reporting.
+  
+const char* ApacheProcessContext::CheckProcessScope(
+  
+    const cmd_parms* cmd, bool* is_process_scope) {
+  
+  VhostCommandHandlingMap::const_iterator p =
+  
+      vhost_command_handling_map_.find(cmd->cmd);
+  
+  *is_process_scope = (p != vhost_command_handling_map_.end());
  const char* ret = NULL;
  if (cmd->server->is_virtual || (cmd->directive->data != NULL)) {
-  
-    VhostCommandHandlingMap::const_iterator p =
-  
-        vhost_command_handling_map_.find(cmd->cmd);
-  
-    if (p != vhost_command_handling_map_.end()) {
+  
+    if (*is_process_scope) {
      ret = CheckGlobalOption(cmd, p->second, factory_->message_handler());
    }
  }
@@ -1376,20 +1385,36 @@ static const char* ParseDirective(cmd_parms* cmd, void* data, const char* arg) {
  if (directive.starts_with(prefix)) {
    StringPiece option = directive.substr(prefix.size());
    GoogleString msg;
+  
+
+  
+    bool use_global_config = false;
+  
+    // See if it's a global option, and perhaps not in place.
+  
+    ret = apache_process_context.CheckProcessScope(cmd, &use_global_config);
+  
+    if (ret != NULL) {
+  
+      return ret;
+  
+    }
+  
+    // Options that are per-process are always parsed into
+  
+    // ApacheRewriteDriverFactory::default_options(), and then propagated
+  
+    // in the post-config hook (pagespeed_post_config).
+  
+    if (use_global_config) {
+  
+      config = ApacheConfig::DynamicCast(factory->default_options());
+  
+    }
+  
+
    // See whether generic RewriteOptions name handling can figure this one out.
    RewriteOptions::OptionSettingResult result =
        config->ParseAndSetOptionFromName1(option, arg, &msg, handler);
    if (result == RewriteOptions::kOptionNameUnknown) {
      // RewriteOptions didn't know; try the driver factory.
+  
+      // TODO(morlovich): It may be cleaner to not have process-scope options
+  
+      // in RewriteOptions at all, but rather something RewriteDriverFactory
+  
+      // specific, as long as we can provide a painless way of integrating it
+  
+      // in the server and parsing it (areas where the current manual approach
+  
+      // fails).
      result = factory->ParseAndSetOption1(
          option, arg,
          !cmd->server->is_virtual,  // is_process_scope
          &msg, handler);
    }
    if (StandardParsingHandled(cmd, result, msg, &ret)) {
-  
-      if (ret == NULL) {
-  
-        ret = apache_process_context.CheckCommandForVhost(cmd);
-  
-      }
      return ret;
    }
  }
@@ -1874,6 +1899,7 @@ void* merge_server_config(apr_pool_t* pool, void* base_conf, void* new_conf) {
          new_non_spdy_overlay.release());
    }
  }
+  
+
  return new_conf;
 }
diff --git a/pagespeed/apache/system_test.sh b/pagespeed/apache/system_test.sh
@@ -1143,6 +1143,19 @@ start_test Fetch gzipped, make sure that we have cache compressed at gzip 9.
 URL="$PRIMARY_SERVER/mod_pagespeed_test/invalid.css"
 fetch_until -gzip $URL "wc -c" 27
+  
+if [ "$SECONDARY_HOSTNAME" != "" ]; then
+  
+  start_test Process-scope configuration handling.
+  
+  # Must be the same value in top-level and both vhosts
+  
+  OUT=$($CURL --silent $HOSTNAME/?PageSpeedFilters=+debug)
+  
+  check_from "$OUT" fgrep -q "IproMaxResponseBytes (imrb) 1048576003"
+  
+
+  
+  OUT=$($CURL --silent --proxy $SECONDARY_HOSTNAME http://ps1.example.com)
+  
+  check_from "$OUT" fgrep -q "IproMaxResponseBytes (imrb) 1048576003"
+  
+
+  
+  OUT=$($CURL --silent --proxy $SECONDARY_HOSTNAME http://ps2.example.com)
+  
+  check_from "$OUT" fgrep -q "IproMaxResponseBytes (imrb) 1048576003"
+  
+fi
+  
+
 # Cleanup
 rm -rf $OUTDIR