CSSStyleSheetResource should cache decoded text instead of raw bytes

third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp

Index: third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp b/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
index 72ee750b8984a5cdbd8a84e040f535449bb2458b..515753658491668efd27ba7bf0a2981b93eba50f 100644
--- a/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
+++ b/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
@@ -43,8 +43,11 @@ CSSStyleSheetResource* CSSStyleSheetResource::fetch(FetchRequest& request,
             WebURLRequest::FrameTypeNone);
   request.mutableResourceRequest().setRequestContext(
       WebURLRequest::RequestContextStyle);
-  return toCSSStyleSheetResource(
+  CSSStyleSheetResource* resource = toCSSStyleSheetResource(
       fetcher->requestResource(request, CSSStyleSheetResourceFactory()));
+  if (resource && !request.integrityMetadata().isEmpty())
+    resource->setIntegrityMetadata(request.integrityMetadata());

[Charlie Harrison, 2016/10/05 15:24:48]

It feels like setting the integrity metadata should now be resource type
agnostic (and possible live in ResourceFetcher::requestResource. I don't feel
too strongly about this though.

[jww, 2016/10/06 01:55:04]

I wrote this code a while back, so my memory isn't perfect, but I believe the
complication is with scripts and when the data is actually available to
calculate the integrity is not when you expect, so it has to be done per
resource type.

[Charlie Harrison, 2016/10/06 03:59:08]

I think you're right about calculating the integrity disposition, but I think
caching the integrity metadata is okay here. This logic is copied over from
ScriptResource::fetch.

That being said I'm still not convinced my suggestion is worthwhile :P

[kouhei (in TOK), 2016/10/06 12:02:36]

Let me try this in separate CL. Added a TODO comment here.

+  return resource;
 }
 
 CSSStyleSheetResource* CSSStyleSheetResource::createForTest(
@@ -99,14 +102,20 @@ void CSSStyleSheetResource::didAddClient(ResourceClient* c) {
 
 const String CSSStyleSheetResource::sheetText(
     MIMETypeCheck mimeTypeCheck) const {
-  if (!data() || data()->isEmpty() || !canUseSheet(mimeTypeCheck))
+  if (!canUseSheet(mimeTypeCheck))
     return String();
 
-  if (!m_decodedSheetText.isNull())
+  // Use cached decoded sheet text when available
+  if (!m_decodedSheetText.isNull()) {
+    // We should have the decoded sheet text cached when the resource is fully loaded.
+    DCHECK_EQ(getStatus(), Resource::Cached);
+
     return m_decodedSheetText;
+  }
+
+  if (!data() || data()->isEmpty())
+    return String();
 
-  // Don't cache the decoded text, regenerating is cheap and it can use quite a
-  // bit of memory
   return decodedText();
 }
 
@@ -121,10 +130,10 @@ void CSSStyleSheetResource::appendData(const char* data, size_t length) {
 }
 
 void CSSStyleSheetResource::checkNotify() {
-  // Decode the data to find out the encoding and keep the sheet text around
-  // during checkNotify()
-  if (data())
+  // Decode the data to find out the encoding and cache the decoded sheet text.
+  if (data()) {
     m_decodedSheetText = decodedText();
+  }
 
   ResourceClientWalker<StyleSheetResourceClient> w(clients());
   while (StyleSheetResourceClient* c = w.next()) {
@@ -132,9 +141,10 @@ void CSSStyleSheetResource::checkNotify() {
     c->setCSSStyleSheet(resourceRequest().url(), response().url(), encoding(),
                         this);
   }
-  // Clear the decoded text as it is unlikely to be needed immediately again and
-  // is cheap to regenerate.
-  m_decodedSheetText = String();
+
+  // Clear raw bytes as now we have the full decoded sheet text.
+  // We wait until all setCSSStyleSheet to run as SubresourceIntegrity checks require raw bytes.

[Charlie Harrison, 2016/10/05 15:24:48]

Is it possible for setCSSStyleSheet to be run on a sheet after this clear() is
called?

Most cases I can tell will end up loading from memory cache which is safe (we
check that the metadata is equal), so it might be worth a comment.

[jww, 2016/10/05 15:32:49]

nit: "until all" -> "for all"

[kouhei (in TOK), 2016/10/06 12:02:36]

Done.

[kouhei (in TOK), 2016/10/06 12:02:36]

Yes. From CSSStyleSheetResource::didAddClient().
However, we are safe as the only consumer of the data() is
LinkStyle::setCSSStyleSheet, which is guaranteed to be called from
CSSStyleSheetResource::checkNotify().

+  clearData();
 }
 
 void CSSStyleSheetResource::destroyDecodedDataIfPossible() {
@@ -145,6 +155,11 @@ void CSSStyleSheetResource::destroyDecodedDataIfPossible() {
   setDecodedSize(0);
 }
 
+void CSSStyleSheetResource::destroyDecodedDataForFailedRevalidation() {
+  m_decodedSheetText = String();
+  destroyDecodedDataIfPossible();
+}
+
 bool CSSStyleSheetResource::canUseSheet(MIMETypeCheck mimeTypeCheck) const {
   if (errorOccurred())
     return false;