New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding ?PageSpeedFilters=+debug to URL enables other filters #1190

Closed
bessey opened this Issue May 5, 2016 · 16 comments

Comments

Projects
None yet
3 participants
@bessey
Copy link

bessey commented May 5, 2016

Steps to repro:

  1. Add pagespeed RewriteLevel PassThrough; to nginx config.
  2. Navigate to a URL and add ?PageSpeedFilters=+debug to it.

Observe that PageSpeed outputs:

<!-- 
mod_pagespeed on
Filters:
ah  Add Head
cc  Combine Css
jc  Combine Javascript
gp  Convert Gif to Png
jp  Convert Jpeg to Progressive
jw  Convert Jpeg To Webp
mc  Convert Meta Tags
pj  Convert Png to Jpeg
ws  When converting images to WebP, prefer lossless conversions
db  Debug
ei  Cache Extend Images
fc  Fallback Rewrite Css 
if  Flatten CSS Imports
hw  Flushes html
ci  Inline Css
ii  Inline Images
il  Inline @import to Link
ji  Inline Javascript
js  Jpeg Subsampling
rj  Recompress Jpeg
rp  Recompress Png
rw  Recompress Webp
ri  Resize Images
cf  Rewrite Css
jm  Rewrite External Javascript
jj  Rewrite Inline Javascript
cu  Rewrite Style Attributes With Url
cp  Strip Image Color Profiles
md  Strip Image Meta Data
...
-->

If i refresh the page a couple times these extra filters start kicking in too (e.g. URLs start getting rewritten even though I have pagespeed CssPreserveURLs On; in my nginx config.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 6, 2016

Could you post the full pagespeed-related configuration?

@bessey

This comment has been minimized.

Copy link

bessey commented May 6, 2016

Sure:

## GLOBAL PAGESPEED CONFIGURATION
pagespeed FileCachePath                   /home/nps-cache;
pagespeed CreateSharedMemoryMetadataCache /home/nps-cache 66000;
pagespeed MessageBufferSize     1000000;
pagespeed FileCacheSizeKb       2048000;
pagespeed LRUCacheKbPerProcess  16384;
pagespeed LRUCacheByteLimit     32768;

## ADMIN
pagespeed Statistics on;
pagespeed StatisticsLogging on;
pagespeed GlobalAdminPath /pagespeed_admin;


server {
  listen    443 ssl http2;
  server_name  www.example.com;

  #### PAGESPEED CONFIGURATION ####
  pagespeed on;
  # I must always be on, even if no experiments are running, or header based filter modification will not work
  pagespeed RewriteLevel CoreFilters;
  pagespeed EnableFilters defer_javascript,prioritize_critical_css,insert_dns_prefetch;
  # Too conservative. If this is enabled, almost all JS is prevented from being renamed
  pagespeed AvoidRenamingIntrospectiveJavascript off;
  # Without this we can't proxy https resources through PageSpeed
  pagespeed FetchHttps enable;
  # Adds necessary config to produce the resource consistently to the URL, this ensures that if two servers
  # have differing config, the resource is served correctly.
  pagespeed AddOptionsToUrls on;

  pagespeed CssInlineMaxBytes 2500;


  ## MEMCACHED
  pagespeed MemcachedServers "www.memcached.example.com";


  location ~ "^/pagespeed_admin" {
     auth_basic           "go away if foe";
     auth_basic_user_file /usr/local/nginx/conf/htpasswd;
  }

  ## STATIC ASSET OPTIMISATION
  # Load example.com files directly from filesystem
  pagespeed LoadFromFile https://www.example.com/assets "/home/deploy/current/public/assets";
  # Rewrite example.com assets to cloudfront domain in HTML output
  pagespeed MapRewriteDomain d2xlm7m6z1xtnp.cloudfront.net www.example.com;
  pagespeed MapRewriteDomain https://d2xlm7m6z1xtnp.cloudfront.net https://www.example.com;
  # Trust example S3 buckets, proxy through PageSpeed
  pagespeed MapProxyDomain "https://www.example.com/s3/example_com_production"
    "https://s3.amazonaws.com/example_com_production"
    "https://d2xlm7m6z1xtnp.cloudfront.net/s3/example_com_production";
  pagespeed MapProxyDomain "https://www.example.com/s3/static_assets"
    "https://example-com-static-assets.s3.amazonaws.com"
    "https://d2xlm7m6z1xtnp.cloudfront.net/s3/static_assets";
  pagespeed MapProxyDomain "https://www.example.com/s3/static_assets_cdn"
    "https://d35dnuw4rnaeot.cloudfront.net"
    "https://d2xlm7m6z1xtnp.cloudfront.net/s3/static_assets_cdn";

  # Ensure requests for pagespeed optimized resources go to the pagespeed handler
  # and no extraneous headers get set.
  location ~ "\.pagespeed\.([a-z]\.)?[a-z]{2}\.[^.]{10}\.[^.]+" {
    add_header "" "";
  }
  location ~ "^/pagespeed_static/" { }
  location ~ "^/ngx_pagespeed_beacon$" { }
  #### END PAGESPEED ####
  ...
}
@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 6, 2016

Confirmed with trunk-tracking and the latest psol.

  • this does not seem specific to enabling the debug filter on the querystring when the server context options are set to passthrough. Enabling any filters seems to toggle enable filters as well.
  • One observation is that enabling the configuration to the server block in configuration does behave as expected - no other filters get enabled.
@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Jul 12, 2016

I suspect this was fixed via (apache/incubator-pagespeed-mod#1305), but this needs to be confirmed

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Jul 19, 2016

Testing this to see if was fixed shows it currently still reproduces using the bleeding edge version.
nginx.conf:

worker_processes  1;
master_process off;
daemon off;

events {
    worker_connections  1024;
}

http {
    include       mime.types;
    default_type  application/octet-stream;

    server {
        listen       8060;
        pagespeed on;
        pagespeed FileCachePath /tmp/psoldef/;
        pagespeed RewriteLevel PassThrough;
        pagespeed EnableFilters debug;

        location / {
            root   html;
            index  index.html index.htm;
        }
    }
}

curl 127.0.0.1 gives:

...
mod_pagespeed on
Filters:
db      Debug
hw      Flushes html
...

But curl 127.0.0.1:8060?PageSpeedFilters=+add_instrumentation enables more then it should:

...
mod_pagespeed on
Filters:
ah      Add Head
ai      Add Instrumentation
cc      Combine Css
jc      Combine Javascript
gp      Convert Gif to Png
jp      Convert Jpeg to Progressive
<snipped more enabled filters>
...
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 20, 2016

I'll have a look at this this morning

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 20, 2016

Some testing:

  • using PageSpeedFilters=debug,add_instrumentation doesn't do this
  • using PageSpeedFilters=-add_instrumentation does do this
  • if you set the page to PassThrough, enable no filers, and change any filter's state through query params, all the core filters apply

For example:

  server {
    pagespeed on;
    listen @@SECONDARY_PORT@@;
    listen [::]:@@SECONDARY_PORT@@;
    server_name debug-filters.example.com;
    pagespeed FileCachePath "@@FILE_CACHE@@";
    pagespeed RewriteLevel PassThrough;
  }

http_proxy=localhost:8051 curl debug-filters.example.com/mod_pagespeed_example/\
rewrite_javascript.html?PageSpeedFilters=-rewrite_css
...
      <script src="rewrite_javascript.js.pagespeed.jm.1o978_K0_L.js" id="ext2">
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 20, 2016

Testing in Apache, with:

<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
  ServerName debug-filters.example.com
  DocumentRoot "@@APACHE_DOC_ROOT@@"
  ModPagespeedFileCachePath "@@MOD_PAGESPEED_CACHE@@"
  ModPagespeedRewriteLevel PassThrough
</VirtualHost>

Confirmed that this does not happen in Apache.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 20, 2016

Looks like something is wrong with how we handle custom options in nginx.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 20, 2016

Calls to cfg_s->server_context->global_options() in nginx look to me like they're getting process-scope and not vhost-scope options, but both GetQueryOptions and ps_determine_options look to me like they're using it expecting to get vhost-scope options. It seems like we should be calling reset_global_options.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 20, 2016

No, I see. The first time you call global_options() on a server context it forks the process-scope options, and in ps_merge_srv_conf we have:

  cfg_s->server_context->global_options()->Merge(*cfg_s->options);

so I think that's not the issue.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 22, 2016

Trying to figure out the difference between how we do this in Nginx and Apache, since it works in Apache. For now I'm going to look only at the case I set up in debug-filters: no experiment, no directory-scope options, just vhost-scope and query-scope.

In apache we have ComputeCustomOptions which does (simplified)

options_ = server_context_->global_config();
directory_aware_options = options_;
server_context_->GetQueryOptions(request_context(),
                                        directory_aware_options,
                                        &stripped_gurl_, request_headers_.get(),
                                        response_headers_.get(),
                                        &rewrite_query_);
query_options = rewrite_query_.options();
# query_options won't be null
# custom_options will be null
custom options_ = NewRewriteOptions();
custom_options_->Merge(*options_);
custom_options_->Merge(*query_options);
options_ = custom_options_.get();

And cutting out even more:

server_context_->GetQueryOptions(server_context_->global_config(), &rewrite_query_);
custom options_ = server_context_->global_config()->Clone();
custom_options_->Merge(rewrite_query_.options());

In nginx, we do:

options = null
global_options = server_context->global_options();
directory_options = null
request_options = ps_determine_request_options(directory_options=null)
server_context->GetQueryOptions(NULL, &rewrite_query)
 -> while we give it NULL, it interprets that to mean server_context->global_options()
have_request_options = true
*options is null
*options = global_options->Clone();
(*options)->Merge(*request_options);

This reduces to:

server_context->GetQueryOptions(server_context->global_options(), &rewrite_query)
*options = global_options->Clone();
(*options)->Merge(*rewrite_query.options);

Hmm. It looks like we're doing the same thing in both cases. I'll try again with the debugger; maybe I'm misreading something.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 22, 2016

Testing nginx in the debugger, I do see ServerContext::global_options() returning a level=PassThrough options.

I'm seeing *options = global_options->Clone() hit on 1528, which I wasn't expecting.

In nginx, when we do (*options)->Merge(*request_options); on 1546, options had been pass through and requestoptions had been core filters, and the output options is core filters.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 22, 2016

In the debugger, what I'm seeing is:

# query_options.level is core filters
# options.level is pass through
options->Merge(*query_options);
# apache: options.level is pass through
# nginx: options.level is core filters
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 22, 2016

At the key merge point in apache:

  • options->level_.was_set() is true
  • query_options->level_.was_set() is false

While in nginx:

  • options->level_.was_set() is true
  • query_options->level_.was_set() has been optimized out ($%^&)
    • actually, all of query_options was optimized out
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 22, 2016

Using OptionsToString however I can confirm my hypothesis: in apache query_options->level_.was_set() is false but in nginx it's true

jeffkaufman added a commit that referenced this issue Jul 22, 2016

jeffkaufman added a commit that referenced this issue Jul 27, 2016

jeffkaufman added a commit that referenced this issue Jul 27, 2016

jeffkaufman added a commit that referenced this issue Aug 2, 2016

jeffkaufman added a commit that referenced this issue Aug 4, 2016

jeffkaufman added a commit to apache/incubator-pagespeed-mod that referenced this issue Aug 4, 2016

crowell added a commit to apache/incubator-pagespeed-mod that referenced this issue Aug 4, 2016

@crowell crowell closed this in 2af2035 Aug 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment