Serializing ResponseHeaders adds Cache Control?

[Jeff Kaufman]

I'm running into something surprising with ResponseHeaders where I see
a Cache-Control header getting added as an effect of serializing.

TEST_F(ResponseHeadersTest, TestRemoveCacheControl) {
ParseHeaders(StrCat("HTTP/1.1 200 (OK)\r\n"
"Cache-Control: max-age=10\r\n"
"Content-Type: text/css\r\n"
"Date: ", start_time_string_, "\r\n\r\n"));

EXPECT_EQ(10 * 1000,
response_headers_.CacheExpirationTimeMs() -
response_headers_.date_ms());

EXPECT_TRUE(response_headers_.Has("Cache-Control"));
EXPECT_STREQ("max-age=10", response_headers_.LookupJoined("Cache-Control"));
EXPECT_TRUE(response_headers_.RemoveAll("Cache-Control"));
EXPECT_FALSE(response_headers_.Has("Cache-Control"));

GoogleString binary_serialization;
StringWriter writer(&binary_serialization);
EXPECT_TRUE(response_headers_.WriteAsBinary(&writer, &message_handler_));
ResponseHeaders deserialized;
EXPECT_TRUE(deserialized.ReadFromBinary(binary_serialization,
&message_handler_));
EXPECT_FALSE(deserialized.Has("Cache-Control")); // this line fails
}

What happens in this test is that RemoveAll does remove the header,
but via UpdateHook it sets cache_fields_dirty. When WriteAsBinary
sees that, it calls ComputeCaching, which calls SetDateAndCaching, and
that sets an actual Cache-Control header. This looks
SetDateAndCaching like it was added to ComputeCaching in
https://github.com/pagespeed/mod_pagespeed/commit/66d9fe9 while
WriteAsBinary always called ComputeCaching from it's first commit
https://github.com/pagespeed/mod_pagespeed/commit/acf18c239cd5680cb09d3e6488d7a8f2e3b337bb
(which is older than the SetDateAndCaching change).

Reading the comments on ComputeCaching it looks like it should just be
updating the pre-computed information that backs the various
ResponseHeaders cache information accessors, not adding new headers.
This means calling ComputeCaching in WriteAsBinary should be both safe
and a no-op.

I'm nervous about "fixing" this, as is, because I don't have a good
sense of how it's supposed to work. Was ComputeCaching supposed to
only update RequestHeaders internal info and not actually change the
headers? But then why did WriteAsBinary call it?

Jeff

[Jeff Kaufman]

ping, now that Josh is back from vacation

[Joshua Marantz]

It does seem like a bug that the deserialization adds CC.  It should never be an error to call ComputeCaching -- just a potential speed penalty.

I think when we first were building this, we anticipated using it only for cacheable resources -- that a resource that had no CC would not need to be serialized.  So this went unnoticed until now.

[Jeff Kaufman]

Ok, so it sounds like the bug is that ComputeCaching modifies caching
headers. I'll fix that.

[Joshua Marantz]

I didn't mean to imply the bug was in ComputeCaching, but maybe it is.  I thought the bug might be in deserialization.

[Jeff Kaufman]

ComputeCaching can currently modify the caching headers, since 2011:
https://github.com/pagespeed/mod_pagespeed/commit/66d9fe9#diff-e37428460863db0568a4720ecceb50f3R351

I was looking for guidance on whether that was within what
ComputeCaching was allowed to do, and it sounds like it's not.